Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TH2-2806] Initial commit #1

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

[TH2-2806] Initial commit #1

wants to merge 49 commits into from

Conversation

tatianavvedenskaya
Copy link

No description provided.

Copy link
Member

@Nikita-Smirnov-Exactpro Nikita-Smirnov-Exactpro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement the IProtocolHandlerFactory interface that the core part can be able to load and create your handler via SrviceLoader.

@@ -0,0 +1 @@
release_version=0.0.0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
release_version=0.0.0
release_version=0.0.1

Dockerfile Outdated
@@ -0,0 +1,9 @@
FROM gradle:6.6-jdk11 AS build

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FROM gradle:6.6-jdk11 AS build
FROM gradle:7.1.0-jdk11 AS build

Use the same version as gradle-wrapper.properties

*/

plugins {
id 'com.palantir.docker' version '0.25.0'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to migrate to the 0.31.0 version

build.gradle Outdated
Comment on lines 24 to 27
ext {
sharedDir = file("${project.rootDir}/shared")
sailfishVersion = '3.2.1594'
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exclude redundant properties

build.gradle Outdated
sailfishVersion = '3.2.1594'
}

group 'org.example'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
group 'org.example'
group 'com.exactpro.th2'


private var serverFuture: Future<*>? = null

private lateinit var receivedHeartbeatCommand: Runnable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same advice as for sentHeartbeatCommand.


fun initCommand(){
if(settings.heartbeatInterval <= 0L) {
throw Exception("Heartbeat sending interval must be greater than zero")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw Exception("Heartbeat sending interval must be greater than zero")
error("Heartbeat sending interval must be greater than zero")

private lateinit var streamId: StreamIdEncode

fun initCommand(){
if(settings.heartbeatInterval <= 0L) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Execute all checks in the init block to verify all incoming parameters only once at constructing an instance of a class.
https://kotlinlang.org/docs/classes.html#constructors


private lateinit var receivedHeartbeatCommand: Runnable

private lateinit var streamId: StreamIdEncode

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use @volatile annotation for a variable with immutable value
https://www.baeldung.com/kotlin/volatile-properties

class PillarHandler(private val channel: Channel): IProtocolHandler {
private val logger = KotlinLogging.logger {}

private var state = State.SESSION_CLOSE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the AtomicReference for this class variable. It will be useful to control switching between states using compareAnd... methods in concurrent execution.

…PillarHandler, PillarMessageDecoder, TestHandler, add PillarHandlerFactory
Copy link

@cordwelt cordwelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review WIP

class PillarHandler(private val channel: Channel,
private val settings: PillarHandlerSettings): IProtocolHandler {
companion object {
private val logger = KotlinLogging.logger { }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private val logger = KotlinLogging.logger { }
private val LOGGER = KotlinLogging.logger {}

import java.util.concurrent.TimeUnit
import java.util.concurrent.atomic.AtomicReference

class PillarHandler(private val channel: Channel,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class PillarHandler(private val channel: Channel,
class PillarHandler(private val channel: IChannel,

Comment on lines 26 to 27
override val name: String
get() = PillarHandlerSettings::class.java.name

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
override val name: String
get() = PillarHandlerSettings::class.java.name
override val name: String = PillarHandlerSettings::class.java.name

Comment on lines 29 to 30
override val settings: Class<out IProtocolHandlerSettings>
get() = PillarHandlerSettings::class.java

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
override val settings: Class<out IProtocolHandlerSettings>
get() = PillarHandlerSettings::class.java
override val settings: Class<out IProtocolHandlerSettings> = PillarHandlerSettings::class.java

get() = PillarHandlerSettings::class.java

override fun create(context: IContext<IProtocolHandlerSettings>): IProtocolHandler {
return PillarHandler(context.channel as Channel, context.settings as PillarHandlerSettings)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return PillarHandler(context.channel as Channel, context.settings as PillarHandlerSettings)
return PillarHandler(context.channel, context.settings as PillarHandlerSettings)

channel.open()

val login = Login(settings)
channel.send(login.login(), messageMetadata(MessageType.LOGIN), IChannel.SendMode.MANGLE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

messageMetadata(MessageType.LOGIN)

Can be cached alongside login object (or inside of it)

Comment on lines 84 to 85
val messageType = buffer.readShort().toInt()
val messageLength = buffer.readShort().toInt()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use must check that there're enough readable bytes before reading something from buffer


if (!MessageType.contains(messageType)){
buffer.resetReaderIndex()
error ("Message type is not supported: $messageType")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should skip the rest of the message, log error and return null instead (or maybe message data). Throwing exception from here would cause channel closure and subsequential reopening

return null
}

if(bufferLength > messageLength) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(bufferLength > messageLength) {
if(bufferLength >= messageLength) {

Comment on lines 100 to 103
val buf: ByteBuf = Unpooled.buffer(messageLength)
buffer.resetReaderIndex()
buf.writeBytes(buffer, 0, messageLength)
buffer.readerIndex(messageLength)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slice of the current buffer can be returned instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants