fix managerName to use pekko
#587
Conversation
|
@pjfanning Does this need a Pekko 1.0.2 release? |
|
I suspect that we are better off not changing this in Pekko 1.0.x. This line in the deprecated classic remoting code is the one that worries me (a little). It may even be best to just to leave this as is. The name ultimately doesn't matter that much and it may not be worth taking the risk that changing it could break something. |
| @@ -136,7 +136,7 @@ private[remote] class PekkoProtocolTransport( | |||
| } | |||
|
|
|||
| override val maximumOverhead: Int = PekkoProtocolTransport.PekkoOverhead | |||
| protected def managerName = s"akkaprotocolmanager.${wrappedTransport.schemeIdentifier}${UniqueId.getAndIncrement}" | |||
| protected def managerName = s"pekkoprotocolmanager.${wrappedTransport.schemeIdentifier}${UniqueId.getAndIncrement}" | |||
There was a problem hiding this comment.
can this be pekko-protocol-manager ?
There was a problem hiding this comment.
I want to maintain the style of the legacy name which is akkaprotocolmanager. I don't think adding - chars makes them a great deal more readable.
|
@pjfanning I have been thinking about this and I do believe that some version of these changes need to be put into Pekko 1.0.x. This is due to the fact that Pekko 1.0.x primary goal is acting as a bridge from Akka to Pekko 1.1.x and so if we merge this PR only into 1.1.x we can unintentionally break users. I also am not sure how much of a problem it would be if such a change would be put into the Pekko 1.0.x branch. As far as I understand this is for protocol changes for the management of the cluster, not the cluster itself which means that you should be able to easily update from a Pekko 1.0.0/1.0.1 cluster to a Pekko cluster with this fix without any real issue (someone needs to comment on this). If this is not the case then this PR for Pekko 1.1.x is fine but we would still need to add code into Pekko 1.0.x to provide a migration path in addition to changing the manager name. I do however strongly think that we should fix this, I don't want the codebase having stray references to Akka and it may be that if none of our users are using the classic remoting that it makes sense to fix this ASAP and just advertise people to use Pekko 1.0.2 |
|
So far, this appears to just be to affect the name of an actor. I need to set up a cluster with Pekko nodes that have this change and ones that don't - to ensure they still work together. |
|
@pjfanning Do you have time to look into this or should I? |
|
@mdedetrich have a look if you like. I think this only affects the name of an actor and is unlikely to affect the ability of pekko instances with this change to communicate with pekko instances that do not have this change. |
pjfanning
force-pushed
the
akka-naming
branch
from
15069e0 to
2b89026
Compare
pjfanning
dismissed stale reviews from He-Pin and mdedetrich
code has changed
|
my preference is to leave this in 1.1 for a while before possibly backporting later. There are 1 or 2 small fixes on 1.0.x branch so a 1.0.3 release might be a good idea soon. I might try to backport when we start discussing 1.0.3. |
* fix managerName to use `pekko` * make name configurable * Update PekkoProtocolTransport.scala
another stray reference to akka
I've made the value configurable, just in case this causes unforeseen issues. Users shouldn't need to change the value.
I have tested with a cluster of 2 Pekko remote actors instances on 2 different remote actor-systems - both configured with different managerNames. The remote actors worked fine together.
The manager name only seems to affect the name of the manager actor instances but not in a way that affects messaging them.