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

add default for pekko.remote.akka.version #1112

Merged
merged 7 commits into from
Feb 12, 2024
Merged

add default for pekko.remote.akka.version #1112

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
29d30b9
add default for pekko.cluster.akka.version
pjfanning Feb 10, 2024
d3fa501
refactor configs
pjfanning Feb 10, 2024
49b89f0
Update reference.conf
pjfanning Feb 10, 2024
f71e8fb
add validations for config settings
pjfanning Feb 11, 2024
10dd71e
Update RemoteSettings.scala
pjfanning Feb 11, 2024
086e891
Update RemoteSettingsSpec.scala
pjfanning Feb 11, 2024
5174d53
scalafmt
pjfanning Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ private[cluster] abstract class SeedNodeProcess(joinConfigCompatChecker: JoinCon
val cfg = context.system.settings.config
if (cfg.hasPath("akka.version")) {
cfg.getString("akka.version")
} else {
} else if (cfg.hasPath("pekko.cluster.akka.version")) {
cfg.getString("pekko.cluster.akka.version")
} else {
"2.6.21"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be configurable as well or is that taking things too far?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could set it in pekko-remote test conf too. Should I just then throw an exception if it isn't set. This code path only happens when pekko.remote.accept-protocol-names is set up to include "akka".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely don't think another config setting improves this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually - I think this could be worse than I thought

This is in our main pekko-remote reference.conf. I don't think this should be in the reference.conf - fine for tests but not set up as our actual default. I think users should enable it in their confs.

    # When receiving requests from other remote actors, what are the valid
    # prefix's to check against. Useful for when dealing with rolling cluster
    # migrations with compatible systems such as Lightbend's Akka.
    accept-protocol-names = ["pekko", "akka"]

Copy link
Contributor

@mdedetrich mdedetrich Feb 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess fundamentally to me it just feels weird that we have a hardcoded akka constant version in the source code considering that the general premise behind all of these "pekko spoofing itself so it looks like akka" values are configurable via typesafe config.

It may also be a stretch but I guess someone could come up with some convoluted scenarios where they don't want the 2.6.21 but something else (tbh I am not that familiar with this code path so ignore it if its a stupid concern).

Copy link
Contributor

@mdedetrich mdedetrich Feb 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is in our main pekko-remote reference.conf. I don't think this should be in the reference.conf - fine for tests but not set up as our actual default. I think users should enable it in their confs.

    # When receiving requests from other remote actors, what are the valid
    # prefix's to check against. Useful for when dealing with rolling cluster
    # migrations with compatible systems such as Lightbend's Akka.
    accept-protocol-names = ["pekko", "akka"]

Whats the problem with having this as a default, seems sensible to me or am I missing something wild? Or are you suggesting just having accept-protocol-names = ["pekko"] (which is something I can get behind) but it still doesn't solve the akka version string issue which is a bit of a gotcha (at minimum it should be documented).

Copy link
Contributor Author

@pjfanning pjfanning Feb 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mdedetrich I might be able to remove this PR if we can agree to change pekko-remote main reference.conf to

    # When receiving requests from other remote actors, what are the valid
    # prefixes to check against. Useful for when dealing with rolling cluster
    # migrations with compatible systems such as Lightbend's Akka.
    # By default, we only support "pekko" protocol.
    # If you want to also support Akka, change this config to:
    # pekko.remote.accept-protocol-names = ["pekko", "akka"]
    accept-protocol-names = ["pekko"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in our main pekko-remote reference.conf. I don't think this should be in the reference.conf - fine for tests but not set up as our actual default. I think users should enable it in their confs.

    # When receiving requests from other remote actors, what are the valid
    # prefix's to check against. Useful for when dealing with rolling cluster
    # migrations with compatible systems such as Lightbend's Akka.
    accept-protocol-names = ["pekko", "akka"]

Whats the problem with having this as a default, seems sensible to me or am I missing something wild? Or are you suggesting just having accept-protocol-names = ["pekko"] (which is something I can get behind) but it still doesn't solve the akka version string issue which is a bit of a gotcha (at minimum it should be documented).

The problem is performance for one. When you allow Akka, you get all the new logic that I enabled to change the compat messages to include akka data as well as pekko data. This is not free.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdedetrich I might be able to remove this PR if we can agree to change pekko-remote main reference.conf to

    # When receiving requests from other remote actors, what are the valid
    # prefixes to check against. Useful for when dealing with rolling cluster
    # migrations with compatible systems such as Lightbend's Akka.
    # By default, we only support "pekko" protocol.
    # If you want to also support Akka, change this config to:
    # pekko.remote.accept-protocol-names = ["pekko", "akka"]
    accept-protocol-names = ["pekko"]

Thats fine with me, but if we document that you need to change it to accept-protocol-names = ["akka", "pekko"] for the migration scenario we then hit the akka version gotcha, so what should we do there? Should we still add a default for pekko.cluster.akka.version even if its only going to be used in the migration scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - I can add pekko.cluster.akka.version to the pekko-remote reference.conf, instead of hardcoding the default in the code.

}
}

Expand Down
Loading