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 2023/08/23 20:54:39 UTC
[GitHub] [incubator-pekko] pjfanning opened a new pull request, #587: fix managerName to use `pekko`
pjfanning opened a new pull request, #587:
URL: https://github.com/apache/incubator-pekko/pull/587
(no comment)
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #587: fix managerName to use `pekko`
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1690678366
Does this need a Pekko 1.0.2 release?
--
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] fix managerName to use `pekko` [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1762777026
@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.
--
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
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #587: fix managerName to use `pekko`
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#discussion_r1303722750
##########
remote/src/main/scala/org/apache/pekko/remote/transport/PekkoProtocolTransport.scala:
##########
@@ -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}"
Review Comment:
can this be `pekko-protocol-manager` ?
--
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] fix managerName to use `pekko` [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1893691832
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.
--
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
[GitHub] [incubator-pekko] pjfanning commented on pull request #587: fix managerName to use `pekko`
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1702433470
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.
--
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] fix managerName to use `pekko` [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #587: fix managerName to use `pekko`
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1702276993
@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.
--
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] fix managerName to use `pekko` [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#discussion_r1451789644
##########
remote/src/main/scala/org/apache/pekko/remote/transport/PekkoProtocolTransport.scala:
##########
@@ -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}"
Review 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.
--
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
[GitHub] [incubator-pekko] pjfanning commented on pull request #587: fix managerName to use `pekko`
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1690692979
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).
```
private def registerManager(): Future[ActorRef] =
(system.actorSelection("/system/transports") ? RegisterTransportActor(managerProps, managerName)).mapTo[ActorRef]
```
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.
--
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] fix managerName to use `pekko` [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #587:
URL: https://github.com/apache/incubator-pekko/pull/587#issuecomment-1762171589
@pjfanning Do you have time to look into this or should I?
--
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