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

Allow cluster formation with mixed protocols #1567

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

sadekmunawar
Copy link
Contributor

Rolling upgrade from Akka to Pekko requires cluster formation with mixed protocol nodes. If the config accept-protocol-names contains both "akka" and "pekko", nodes using either protocol should be able join the same cluster.

Relates to #765

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.

thanks @sadekmunawar - this looks like a necessary change

@pjfanning pjfanning added this to the 1.1.3 milestone Nov 27, 2024
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Another great observation, and thanks for also adding a test!

@raboof raboof merged commit e4fa6f5 into apache:main Nov 27, 2024
9 checks passed
@raboof
Copy link
Member

raboof commented Nov 27, 2024

(I'll backport)

@pjfanning
Copy link
Contributor

@sadekmunawar do you think doing a release with your changes is a good idea or are you in the middle of testing mixed clusters and feel like you might find more issues?

@sadekmunawar
Copy link
Contributor Author

@sadekmunawar do you think doing a release with your changes is a good idea or are you in the middle of testing mixed clusters and feel like you might find more issues?

@pjfanning I had to make one more change for the migration. I'll create a PR by the end of the week. Once that's merged, I think a release can be done.

@pjfanning
Copy link
Contributor

@sadekmunawar any news on progress? If you don't have time to create a PR for the remaining issue, could you give us some pointers so someone else could create the PR instead?

@sadekmunawar
Copy link
Contributor Author

sadekmunawar commented Dec 10, 2024

@pjfanning There are two missing pieces left:

  1. Deserialization of Akka cluster messages. I created the PR for this one Enable deserialization of old Akka cluster messages (mixed pekko/akka cluster) #1578
  2. Supporting Akka config for JoinConfigCompatChecker. An exception is thrown here, when a Pekko node tries to find the downing path in the Akka config. I solved it by simply checking if the path exists in the config before doing the comparison.
    val downingProviderResult = {
      if (toCheck.hasPath(DowningProviderPath) && actualConfig.hasPath(DowningProviderPath))
        JoinConfigCompatChecker.checkEquality(List(DowningProviderPath), toCheck, actualConfig)
      else Invalid(im.Seq("Downing path not found. Set configuration-compatibility-check.enforce-on-join to off to enable cluster joining."))
    }

While this all that's necessary for the rolling upgrade support, I tried to find a better approach for the second problem. See this commit. I haven't had the chance to integration test this approach, yet. It should fix the exception issue, but it likely sends configs with Pekko paths to Akka nodes here

@pjfanning
Copy link
Contributor

pjfanning commented Dec 16, 2024

I am going to revert this due to #1586

A build of #1587 with the nightly builds temporarily enabled shows that this PR is the cause.

We can try to fix this issue again but we need a working build first - #1590.

@pjfanning pjfanning removed this from the 1.1.3 milestone Dec 16, 2024

import org.apache.pekko.testkit.{ LongRunningTest, PekkoSpec }

object MixedProtocolClusterSpec {
Copy link
Contributor

Choose a reason for hiding this comment

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

this test doesn't work in the nightly build CI job and the change breaks many other cluster tests too

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

Successfully merging this pull request may close these issues.

3 participants