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.

Sign up for GitHub

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

Jump to bottom

!test Migrate multi node testkit to Netty 4. #539

Merged
merged 1 commit into from
Aug 6, 2023
Merged

!test Migrate multi node testkit to Netty 4. #539

merged 1 commit into from
Aug 6, 2023

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Aug 5, 2023 •
edited
Loading

refs: #462

I verified it locally with sbt cluster/MultiJvm/test
previous: #486

He-Pin
He-Pin commented Aug 5, 2023
View reviewed changes
project/SbtMultiJvm.scala Outdated Show resolved Hide resolved
@He-Pin He-Pin requested a review from mdedetrich August 5, 2023 09:26
@He-Pin He-Pin added this to the 1.1.0 milestone Aug 5, 2023
@He-Pin He-Pin requested a review from pjfanning August 5, 2023 09:26
@pjfanning
Copy link
Contributor

I agree that this can be merged to pekko main (when it is approved) and eventually released with pekko 1.1.0.

I think that anyone who needs this code before the pekko 1.1.0 can easily create their own jar. It would probably be useful for a pekko-multi-node-netty4-testkit project to be created in GitHub for that project to release jars. The project can be archived after the pekko 1.1.0 release.

pjfanning
pjfanning requested changes Aug 5, 2023
View reviewed changes
project/SbtMultiJvm.scala Outdated Show resolved Hide resolved
@He-Pin
Copy link
Member Author

He-Pin commented Aug 5, 2023

@pjfanning ping and telnet is not available on Windows, then I need to implement another telnet in java to test the controller is started up.

@pjfanning
Copy link
Contributor

@pjfanning ping and telnet is not available on Windows, then I need to implement another telnet in java to test the controller is started up.

When I say ping, I don't mean the Unix command. Wiktionary has this meaning (among many) of the word 'ping'.

A packet which a remote host is expected to echo, thus indicating its presence.

@He-Pin
Copy link
Member Author

He-Pin commented Aug 5, 2023 •
edited
Loading

I will udpate the pr wit a java ping then, but I still think the current sleep is a quick fix than before.

@He-Pin He-Pin requested a review from pjfanning August 5, 2023 13:34
@He-Pin He-Pin mentioned this pull request Aug 5, 2023
Closed
@He-Pin He-Pin force-pushed the netty4 branch 3 times, most recently from 8e3d77b to 3d9c26f Compare August 5, 2023 17:39
mdedetrich
mdedetrich requested changes Aug 6, 2023
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Great work, PR generally looks fine. My main remarks are regarding magic constants and some changes I think be split out and merged on their own so they can be backported.

multi-node-testkit/src/main/scala/org/apache/pekko/remote/testconductor/RemoteConnection.scala Outdated Show resolved Hide resolved
cluster/src/multi-jvm/resources/logback-test.xml Show resolved Hide resolved
multi-node-testkit/src/main/scala/org/apache/pekko/remote/testconductor/RemoteConnection.scala Outdated
override def shutdown(): Unit = {
try {
channelFuture.channel().close().sync()
parentEventLoopGroup.shutdownGracefully(1L, 1L, TimeUnit.SECONDS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment about configuration

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @mdedetrich - this would be better as a setting in reference.conf

Copy link
Member Author

Choose a reason for hiding this comment

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

I just updated the code without this, but which may waiting max to 15 seconds. the old code is deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mdedetrich I updated it to .shutdownGracefully(0L, 0L, TimeUnit.SECONDS) which alian the old behavior.

project/SbtMultiJvm.scala Outdated Show resolved Hide resolved
project/SbtMultiJvm.scala Show resolved Hide resolved
project/SbtMultiJvm.scala Show resolved Hide resolved
@mdedetrich
Copy link
Contributor

mdedetrich commented Aug 6, 2023 •
edited
Loading

I have just run the cluster tests on the latest version of this PR on my linux x86_64 box and I can confirm that they all pass, good work! After the comments are addressed the PR looks to be in good shape to me.

I have attached the test log for those curios

multi-jvm-test-main-6.txt

mdedetrich
mdedetrich approved these changes Aug 6, 2023
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

So I am approving changes, previous comments are resolved. I am not fully aware of this part of the code so it would be good to get some more eyes before merging (we aren't in a massive rush to merge this right now).

He-Pin
He-Pin commented Aug 6, 2023
View reviewed changes
project/SbtMultiJvm.scala
val testClass2Process = (testClass, Jvm.startJvm(javaBin, allJvmOptions, runOptions, jvmLogger, connectInput))
if (index == 0) {
log.debug("%s for %s 's started as `Controller`, waiting before can be connected for clients.".format(jvmName,
testClass))
Copy link
Member Author

@He-Pin He-Pin Aug 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@mdedetrich This is the real fix which can be a sleep(2000) or like thing. the error was a CONNECT(2) from the networking , which I was thing caused by resolver or my code bug.

@He-Pin He-Pin force-pushed the netty4 branch 2 times, most recently from b772a9b to 5a4b65f Compare August 6, 2023 11:49
@He-Pin He-Pin requested a review from mdedetrich August 6, 2023 11:57
@He-Pin
!test Migrate multi node testkit to Netty 4.
d4c436f 
Signed-off-by: He-Pin <hepin1989@gmail.com>
pjfanning
pjfanning approved these changes Aug 6, 2023
View reviewed changes
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin
Copy link
Member Author

He-Pin commented Aug 6, 2023 •
edited
Loading

I ran several times on gitpod. it works, I will do a final validation before merge this.

@He-Pin
Copy link
Member Author

He-Pin commented Aug 6, 2023 •
edited
Loading

I ran it several times again and it do passed, so it should be good to go.

@He-Pin He-Pin merged commit 0e89e79 into apache:main Aug 6, 2023
17 checks passed
@He-Pin He-Pin deleted the netty4 branch September 9, 2023 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjfanning pjfanning pjfanning approved these changes

@mdedetrich mdedetrich Awaiting requested review from mdedetrich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.0-M1
Development

Successfully merging this pull request may close these issues.
3 participants
@He-Pin @pjfanning @mdedetrich