You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "pjfanning (via GitHub)" <gi...@apache.org> on 2024/02/10 11:12:59 UTC
[PR] add default for pekko.cluster.akka.version [incubator-pekko]
pjfanning opened a new pull request, #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485091228
##########
remote/src/main/resources/reference.conf:
##########
@@ -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"
Review 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#issuecomment-1938536716
> Is there already an issue for the missing nightly build schedule in `1.0.x`? Otherwise, looks good to me
I logged https://github.com/apache/incubator-pekko/issues/1119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485071016
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485069325
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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"]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485066414
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review Comment:
I definitely don't think another config setting improves this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485105165
##########
remote/src/main/resources/reference.conf:
##########
@@ -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"
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485067542
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485067235
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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"]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485071664
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review Comment:
Sure - I can add `pekko.cluster.akka.version` to the pekko-remote reference.conf, instead of hardcoding the default in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485068520
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485068520
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485070192
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485069325
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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"]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485068520
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485289916
##########
remote/src/main/resources/reference.conf:
##########
@@ -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"
Review 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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485067542
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485066347
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review 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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] add default for pekko.cluster.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485065506
##########
cluster/src/main/scala/org/apache/pekko/cluster/SeedNodeProcess.scala:
##########
@@ -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"
Review Comment:
Should this be configurable as well or is that taking things too far?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#issuecomment-1936988304
Looks much better now, let me know when PR is out of draft and ill review it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] [DRAFT] add default for pekko.remote.akka.version [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1112:
URL: https://github.com/apache/incubator-pekko/pull/1112#discussion_r1485091228
##########
remote/src/main/resources/reference.conf:
##########
@@ -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"
Review 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org