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
+70 −9
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
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
2 changes: 1 addition & 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 @@ -58,7 +58,7 @@ private[cluster] abstract class SeedNodeProcess(joinConfigCompatChecker: JoinCon
if (cfg.hasPath("akka.version")) {
cfg.getString("akka.version")
} else {
cfg.getString("pekko.cluster.akka.version")
cfg.getString("pekko.remote.akka.version")
}
}

Expand Down
13 changes: 11 additions & 2 deletions remote/src/main/resources/reference.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,12 @@ pekko {
warn-unsafe-watch-outside-cluster = on

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

# The protocol name to use when sending requests to other remote actors.
# Useful when dealing with rolling migration, i.e. temporarily change
Expand All @@ -198,6 +201,12 @@ pekko {
# nodes have been are running on Apache Pekko
protocol-name = "pekko"

# When pekko.remote.accept-protocol-names contains "akka", then we
# 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"
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 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

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)?


# Settings for the Phi accrual failure detector (http://www.jaist.ac.jp/~defago/files/pdf/IS_RR_2004_010.pdf
# [Hayashibara et al]) used for remote death watch.
# The default PhiAccrualFailureDetector will trigger if there are no heartbeats within
Expand Down
Loading