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-4953] - th2 transport protocol #7

Merged
merged 10 commits into from
Sep 15, 2023
Merged

[TH2-4953] - th2 transport protocol #7

merged 10 commits into from
Sep 15, 2023

Conversation

lumber1000
Copy link
Member

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.

Please update readme

core/build.gradle Outdated Show resolved Hide resolved
core/build.gradle Show resolved Hide resolved
val publication: PublicationConfiguration = PublicationConfiguration()
val publication: PublicationConfiguration = PublicationConfiguration(),
val useTransport: Boolean = false,
val sessionGroup: String
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that the single session group should be used for the whole read-db. If we want to add we need to add the ability to set the group per data source.
Otherwise, it doesn't really make sense (at least for me).
@Nikita-Smirnov-Exactpro what do you think about the current implementation and my opinion?

private fun TableRow.toTransportMessage(dataSourceId: DataSourceId): RawMessage.Builder {
val builder = RawMessage.builder()
.setBody(Unpooled.wrappedBuffer(toCsvBody()))
.setId(MessageId(dataSourceId.id, Direction.INCOMING, 0, Instant.EPOCH))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use idBuilder here (because we cannot set sequence and timestamp here)

}
}

private data class SessionKey<DIRECTION>(val alias: String, val direction: DIRECTION)
Copy link
Member

Choose a reason for hiding this comment

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

I we use groups here the group must be used for the session key instead of the alias

version update
README.md update
@OptimumCode
Copy link
Member

Looks okay to me.
@Nikita-Smirnov-Exactpro What do you think?

Comment on lines +364 to +369
sequences.compute(key) { _, prev ->
if (prev == null) {
Instant.now().run { epochSecond * nanosInSecond + nano }
} else {
prev + 1
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be moved to the parent class

@OptimumCode
Copy link
Member

@lumber1000 Are you okay to merge the changes?

Oleg Smelov and others added 3 commits August 2, 2023 10:54
core/build.gradle Show resolved Hide resolved
@@ -12,7 +14,7 @@ ext {
coroutines_version = '1.6.4'
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to update the coroutines version because we have updated the kotlin version

version update
dev release workflow added
@OptimumCode OptimumCode self-requested a review August 31, 2023 10:53
- versions
uses: th2-net/.github/.github/workflows/compaund-python-grpc-pypi-publication.yml@main
with:
custom-version: ${{ needs.versions.outputs.version }}-dev
Copy link
Member

Choose a reason for hiding this comment

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

Please, use .rc1 suffix instead of -dev

uses: th2-net/.github/.github/workflows/compaund-java-docker-push.yml@main
with:
docker-username: ${{ github.actor }}
version: ${{ needs.versions.outputs.version }}-dev
Copy link
Member

Choose a reason for hiding this comment

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

From which project this version is taken? By default th2-net/.github/.github/workflows/compound-prebuild-java-workflow.yml@main takes the version from the root project. But this is not what we won't. The version should be taken from app project. You need to specify the project path to get the right version

build.gradle Outdated
id "org.owasp.dependencycheck" version "8.1.2"
id "io.github.gradle-nexus.publish-plugin" version "1.3.0"
id 'org.jetbrains.kotlin.jvm' version '1.8.22' apply false
id "org.owasp.dependencycheck" version "8.3.1"
Copy link
Member

Choose a reason for hiding this comment

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

Please, add plugins for licenses and git properties

Copy link
Member

Choose a reason for hiding this comment

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

Please, take a look at other grpc repos (check1 for example - the latest dev tag) and correct the sourceSets block

@@ -31,4 +37,21 @@ dependencyCheck {

dependencyLocking {
lockAllConfigurations()
}

licenseReport {
Copy link
Member

Choose a reason for hiding this comment

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

Does this plugin work for subproject when you apply it to the root project? Could you please check that?

@lumber1000 lumber1000 merged commit feb1a0c into dev-2 Sep 15, 2023
11 checks passed
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