add default for pekko.remote.akka.version #1112
Conversation
| cfg.getString("pekko.cluster.akka.version") | ||
| } else { | ||
| "2.6.21" |
There was a problem hiding this comment.
Should this be configurable as well or is that taking things too far?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
I definitely don't think another config setting improves this.
There was a problem hiding this comment.
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"]
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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"]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Sure - I can add pekko.cluster.akka.version to the pekko-remote reference.conf, instead of hardcoding the default in the code.
pjfanning
changed the title
pjfanning
changed the title
|
Looks much better now, let me know when PR is out of draft and ill review it. |
| # need to know the Akka version. If you include the Akka jars on the classpath, | ||
| # we can use the akka.version from their configuration. This configuration | ||
| # setting is only used if we can't find an akka.version setting. | ||
| akka.version = "2.6.21" |
There was a problem hiding this comment.
This might be over-engineering way too much but considering you could put any string values into accept-protocol-names it might make sense for this configuration to be an object i.e.
version {
akka = "2.6.21"
}
where the key (in this case akka) is pointing to the the same value inside of accept-protocol-names.
Otherwise we should do some sanity validation on accept-protocol-names and refuse any values that aren't akka or pekko
There was a problem hiding this comment.
I think this is overengineering. The new code I added is very Akka specific (#765). It only kicks if you include "akka" in accept-protocol-names (again hardcoded). In the real world, I do not know of anyone else who would need to add a non-Pekko node to a Pekko cluster. If that story ever comes to pass, I will volunteer to help those users to get the cluster compatibility checks to work with their nodes.
There was a problem hiding this comment.
Can we then just add a validation check to the config parsing so that only akka and pekko can be put in as values (can also be separate PR)?
pjfanning
changed the title
I logged #1119 |
* add default for pekko.cluster.akka.version * refactor configs * Update reference.conf * add validations for config settings * Update RemoteSettings.scala * Update RemoteSettingsSpec.scala * scalafmt
* Handle mixed akka/pekko protocol names * add extra changes needed to get akka cluster support * add default for pekko.remote.akka.version (#1112) * add default for pekko.cluster.akka.version * refactor configs * Update reference.conf * add validations for config settings * Update RemoteSettings.scala * Update RemoteSettingsSpec.scala * scalafmt --------- Co-authored-by: Matthew de Detrich <[email protected]>
This property is only needed if you have pekko.remote.accept-protocol-names set up to include "akka". This is the case in the test config for pekko-remote and we get a lot of logging about pekko.cluster.akka.version as a result.
Relates to #765 and can only be merged to 1.0.x because #765 is only merged to that branch.
spotted in https://github.com/apache/incubator-pekko/actions/runs/7854077720 - we don't run the nightly tests on 1.0.x branch (and we should probably fix that too)
As part of the change, I have renamed the config to pekko.remote.akka.version so that the config is with the other new configs.