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

reimplement fix for akka/pekko cluster #1594

Merged
merged 15 commits into from
Jan 1, 2025
Merged

reimplement fix for akka/pekko cluster #1594

merged 15 commits into from
Jan 1, 2025

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Dec 16, 2024 •
edited
Loading

Run I the tests locally they pass but the nightly tests were failing until I removed this change.

see #1590

sbt \
            -Dpekko.cluster.assert=on \
            -Dpekko.log.timestamps=true \
            -Dpekko.test.timefactor=2 \
            -Dpekko.actor.testkit.typed.timefactor=2 \
            -Dpekko.test.tags.exclude=gh-exclude,timing \
            -Dpekko.test.multi-in-test=false \
"cluster/testOnly *JoinConfigCompatCheckerRollingUpdateSpec"

@pjfanning pjfanning marked this pull request as draft December 16, 2024 20:47
@pjfanning
Copy link
Contributor Author

@raboof @kerr @sadekmunawar

My changes have fixed most tests but the new ones in this PR are still broken. It looks like we need more changes in org.apache.pekko.remote.Remoting$.localAddressForRemote.

Error:  [12/16/2024 22:04:04.724] [MixedProtocolClusterSpec-pekko.actor.internal-dispatcher-4] [org.apache.pekko.cluster.ClusterActorRefProvider] No root guardian at [pekko.tcp://MixedProtocolClusterSpec@10.1.0.6:40819]
org.apache.pekko.remote.RemoteTransportException: No transport is loaded for protocol: [pekko.tcp], available protocols: [akka.tcp]
	at org.apache.pekko.remote.Remoting$.localAddressForRemote(Remoting.scala:125)
	at org.apache.pekko.remote.Remoting.localAddressForRemote(Remoting.scala:168)
	at org.apache.pekko.remote.RemoteActorRefProvider.rootGuardianAt(RemoteActorRefProvider.scala:488)

@pjfanning pjfanning marked this pull request as ready for review December 17, 2024 12:15
@pjfanning pjfanning marked this pull request as draft December 17, 2024 12:39
@pjfanning
Copy link
Contributor Author

pjfanning commented Dec 17, 2024 •
edited
Loading

@raboof @He-Pin @sadekmunawar I think I have all the tests passing now. I will check again later. If these changes are approved, I can remove the temp change to run the nightly tests during CI build of this PR.

#1610 raised for a flaky test

@He-Pin
Copy link
Member

He-Pin commented Dec 17, 2024

I have not look into the details, but will it replay the akka node with akka protocol ?otherwise, it will not work. can a akka node sending ping-pong after joined？

And there are akka.tcp and akka protocols, which should be tested too.

@pjfanning
Copy link
Contributor Author

@He-Pin this PR is based on #1567 but makes extra changes because the nightly tests failed with the 1567 changes. Specifically, the some nightly tests use pekko.tcp/akka.tcp transports. 1567 did not properly support the .tcp transports. These changes fix that.

@pjfanning pjfanning marked this pull request as ready for review December 21, 2024 15:43
@pjfanning pjfanning changed the title try to find issue with akka/pekko cluster change reimplement fix for akka/pekko cluster Dec 21, 2024
@pjfanning pjfanning added this to the 1.2.0 milestone Dec 28, 2024
He-Pin
He-Pin reviewed Dec 28, 2024
View reviewed changes
cluster/src/test/scala/org/apache/pekko/cluster/MixedProtocolClusterSpec.scala
pekko.coordinated-shutdown.terminate-actor-system = on

pekko.remote.classic.netty.tcp.port = 0
pekko.remote.artery.canonical.port = 0
Copy link
Member

Choose a reason for hiding this comment

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

we need test both pekko.tcp and pekko, I mean both artery and classical transports, seems this only test one.

@pjfanning pjfanning marked this pull request as draft December 28, 2024 16:32
@pjfanning pjfanning mentioned this pull request Dec 28, 2024
Open
@pjfanning
Copy link
Contributor Author

@He-Pin @raboof does this look ok? When it is ok, I can remove the CI change that makes the nightly tests run as part of the PR CI build.

@raboof
Copy link
Member

raboof commented Dec 30, 2024

Looks reasonable to me.

@pjfanning pjfanning marked this pull request as ready for review December 30, 2024 21:16
@pjfanning
Copy link
Contributor Author

I changed the PR tests to include some cluster tests as per #1591

He-Pin
He-Pin reviewed Dec 30, 2024
View reviewed changes
.github/workflows/build-test-prValidation.yml
fail-fast: false
matrix:
command:
- cluster/test distributed-data/test cluster-tools/test cluster-metrics/test
Copy link
Member

Choose a reason for hiding this comment

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

need sbt cluster/MultiJvm/test too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1592 covers that. It looks like running the multiJvm tests will be more complicated. The aim of this small change is that the test modified in this PR runs in the PR CI - because at the momemt, it does not.

He-Pin
He-Pin approved these changes Dec 31, 2024
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Great!
lgtm, it covers pekko join akka and akka join pekko.

@pjfanning pjfanning merged commit ef212e9 into apache:main Jan 1, 2025
9 checks passed
@pjfanning pjfanning deleted the cluster-akka branch January 1, 2025 11:13
pjfanning added a commit to pjfanning/incubator-pekko that referenced this pull request Jan 2, 2025
@pjfanning
reimplement fix for akka/pekko cluster (apache#1594)
b17140a 
* Revert "revert apache#1568 due to test failures (apache#1587)"

This reverts commit 7af03e5.

* temp run nightly test in this PR

* no need for square brackets because the set print adds them

* logging to find issue

* support tcp protocols

* Update ClusterDaemon.scala

* remove temp logging

* try to fix issue in Remoting

* extra tests

* more tests

* ignore udp tests

* try to make tests tidy up after failures

* Update MixedProtocolClusterSpec.scala

* Update MixedProtocolClusterSpec.scala

* run main cluster tests for PR
@pjfanning pjfanning mentioned this pull request Jan 2, 2025
Merged
pjfanning added a commit that referenced this pull request Jan 2, 2025
@pjfanning
reimplement fix for akka/pekko cluster (#1594) (#1664)
38facf7 
* Revert "revert #1568 due to test failures (#1587)"

This reverts commit 7af03e5.

* temp run nightly test in this PR

* no need for square brackets because the set print adds them

* logging to find issue

* support tcp protocols

* Update ClusterDaemon.scala

* remove temp logging

* try to fix issue in Remoting

* extra tests

* more tests

* ignore udp tests

* try to make tests tidy up after failures

* Update MixedProtocolClusterSpec.scala

* Update MixedProtocolClusterSpec.scala

* run main cluster tests for PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@He-Pin He-Pin He-Pin approved these changes

Assignees
No one assigned
Labels
backport
Projects
None yet
Milestone
1.2.0
Development

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