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

!test Migrate akka-multi-node-testkit to Netty4 #32005

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jul 1, 2023

References #31974

Not sort the imports yet.

@He-Pin He-Pin marked this pull request as draft July 1, 2023 13:50
@He-Pin He-Pin force-pushed the netty4 branch 3 times, most recently from cd55137 to cdf5bab Compare July 3, 2023 06:23
@johanandren
Copy link
Member

Huh, something weird with validation, I'll close and re-open to retrigger.

@johanandren johanandren closed this Jul 3, 2023
@johanandren johanandren reopened this Jul 3, 2023
@He-Pin He-Pin marked this pull request as ready for review July 3, 2023 09:08
@He-Pin
Copy link
Member Author

He-Pin commented Jul 3, 2023

@johanandren I think this is ready.

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

Looking good, thanks.

Note that we don't run multi-jvm tests in PR validation, so before merging we should verify locally.

@johanandren
Copy link
Member

I think we can also trigger the multi-jvm tests manually against this branch before merging.

@johanandren
Copy link
Member

Seeing failures with this exception when I run it locally:

 WARNING: Failed to initialize a channel. Closing: [id: 0xe92cb4cd, L:/127.0.0.1:4711 - R:/127.0.0.1:54387]
[error] [JVM-1-LeaderElectionWithFailureDetectorPuppetMultiJvmNode1] io.netty.channel.ChannelPipelineException: akka.remote.testconductor.ConductorHandler is not a @Sharable handler, so can't be added or removed multiple times.
[error] [JVM-1-LeaderElectionWithFailureDetectorPuppetMultiJvmNode1] 	at io.netty.channel.DefaultChannelPipeline.checkMultiplicity(DefaultChannelPipeline.java:600)
[error] [JVM-1-LeaderElectionWithFailureDetectorPuppetMultiJvmNode1] 	at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:202)
[error] [JVM-1-LeaderElectionWithFailureDetectorPuppetMultiJvmNode1] 	at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:195)
[error] [JVM-1-LeaderElectionWithFailureDetectorPuppetMultiJvmNode1] 	at akka.remote.testconductor.TestConductorPipelineFactory.$anonfun$initChannel$1(RemoteConnection.scala:81)
...

@He-Pin
Copy link
Member Author

He-Pin commented Jul 7, 2023

@johanandren I have updated, and add the @Shareable annotation. have very bad network connection, can you check it out again?

@He-Pin He-Pin closed this Jul 10, 2023
@He-Pin He-Pin reopened this Jul 10, 2023
@johanandren
Copy link
Member

Now the multi jvm cluster tests are failing with a different error, connection failed, did you get those to pass locally with these changes? akka-cluster/MultiJvm/test

@He-Pin
Copy link
Member Author

He-Pin commented Jul 10, 2023

Let me check it then.

@He-Pin He-Pin marked this pull request as draft July 11, 2023 02:42
@He-Pin He-Pin marked this pull request as draft July 11, 2023 02:42
@He-Pin He-Pin marked this pull request as ready for review July 11, 2023 16:59
@johanandren
Copy link
Member

Still seeing tests failing with connection refused errors when I try to run cluster multi JVM tests with this

@He-Pin
Copy link
Member Author

He-Pin commented Jul 12, 2023

Still seeing tests failing with connection refused errors when I try to run cluster multi JVM tests with this

hum,let's
me check it this weekend.

@johanandren
Copy link
Member

No urgency, vacation times in Europe, so reviewing will be slow the next couple of weeks anyway.

@He-Pin He-Pin force-pushed the netty4 branch 5 times, most recently from be918b7 to 1744aed Compare July 15, 2023 13:31
@He-Pin He-Pin requested a review from patriknw July 25, 2023 14:46
@He-Pin He-Pin marked this pull request as draft August 1, 2023 12:42
@He-Pin He-Pin marked this pull request as ready for review August 5, 2023 17:06
@He-Pin He-Pin force-pushed the netty4 branch 5 times, most recently from 8142174 to 25a178a Compare August 7, 2023 03:10
@He-Pin He-Pin marked this pull request as draft August 8, 2023 09:34
@He-Pin He-Pin marked this pull request as ready for review August 10, 2023 05:09
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, ran through cluster multi jvm tests on both MacOS and Linux with all tests green with these changes. 👍

Thanks @He-Pin

@He-Pin
Copy link
Member Author

He-Pin commented Aug 14, 2023

@johanandren Yes, it did take me sometime to make it right, and now, Netty 3 is gone.

</root>
<logger name="io.netty.util.Recycler" level="ERROR" />
<logger name="io.netty.buffer.PoolThreadCache" level="ERROR" />
</configuration>
Copy link
Member

Choose a reason for hiding this comment

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

is slf4j/logback needed by Netty or what is the reason we need this now?

is this included by the other sbt projects that have multi-jvm tests or is same needed in more places?

Copy link
Member

Choose a reason for hiding this comment

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

I see this in logs:

2023-08-14T08:44:19.0726537Z �[0m[�[0m�[31merror�[0m] �[0m�[0m[�[34mJVM-2�[0m] SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".�[0m
2023-08-14T08:44:19.0727622Z �[0m[�[0m�[31merror�[0m] �[0m�[0m[�[34mJVM-2�[0m] SLF4J: Defaulting to no-operation (NOP) logger implementation�[0m

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 need it to suppress netty 4's logging

Copy link
Member

Choose a reason for hiding this comment

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

I'll add it for all multi-jvm tests then.

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

I have tried this in CI: https://github.com/akka/akka/actions/runs/5853551428
Seems to work fine, we have some flaky tests since before.

</root>
<logger name="io.netty.util.Recycler" level="ERROR" />
<logger name="io.netty.buffer.PoolThreadCache" level="ERROR" />
</configuration>
Copy link
Member

Choose a reason for hiding this comment

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

I see this in logs:

2023-08-14T08:44:19.0726537Z �[0m[�[0m�[31merror�[0m] �[0m�[0m[�[34mJVM-2�[0m] SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".�[0m
2023-08-14T08:44:19.0727622Z �[0m[�[0m�[31merror�[0m] �[0m�[0m[�[34mJVM-2�[0m] SLF4J: Defaulting to no-operation (NOP) logger implementation�[0m

@patriknw
Copy link
Member

I created issue #32038 but that happened once before this PR as well.

@johanandren johanandren merged commit 3eef04a into akka:main Aug 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants