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.

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

Handle mixed akka/pekko protocol names #765

Merged

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Oct 22, 2023

Resolves: #108

Some additional changes were made in #1112

Described in https://cwiki.apache.org/confluence/display/PEKKO/Pekko+Akka+Compatibility

@mdedetrich mdedetrich marked this pull request as draft October 22, 2023 17:28
@mdedetrich mdedetrich force-pushed the handle-mixed-akka-pekko-protocol-names branch from 0d5bdb2 to 41b6e8a Compare October 22, 2023 17:35
@mdedetrich mdedetrich force-pushed the handle-mixed-akka-pekko-protocol-names branch from 41b6e8a to 9a5c35a Compare October 22, 2023 19:36
@mdedetrich
Copy link
Contributor Author

incubator-pekko/remote/src/main/scala/org/apache/pekko/remote/transport/PekkoProtocolTransport.scala

Thanks pushed, lets see if it breaks anything

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

is the intention to add some akka protocol tests in this PR or maybe in a follow up PR?

remote/src/main/resources/reference.conf Show resolved Hide resolved
@mdedetrich
Copy link
Contributor Author

is the intention to add some akka protocol tests in this PR or maybe in a follow up PR?

The intention is to add the tests in the same PR, hence why its a draft.

@pjfanning pjfanning added this to the 1.0.x milestone Nov 10, 2023
@pjfanning
Copy link
Contributor

@mdedetrich what would you think of merging this soon more or less as is but we document it as experimental. Then we can release v1.0.2 with this and see if we can crowdsource the testing. Alternatively, we add it to main only, for now and release v1.1.0-M1 and again see if we can get people to test with it.

@mdedetrich
Copy link
Contributor Author

@mdedetrich what would you think of merging this soon more or less as is but we document it as experimental. Then we can release v1.0.2 with this and see if we can crowdsource the testing. Alternatively, we add it to main only, for now and release v1.1.0-M1 and again see if we can get people to test with it.

Not really confident merging this as is because @He-Pin said that the issue is more complicated and we also need to do port roll-over to.

@pjfanning
Copy link
Contributor

Ports are configurable already. For now, mixed cluster users can use Akka port numbers. They can even use Akka ports after the whole cluster is Pekko. We can add support for mixed port numbers later.

@pjfanning
Copy link
Contributor

I think this PR is bettered targeted at main branch. We can backport it when we have something that we are confident in. We can merge experimental or partial impls to main.

@mdedetrich
Copy link
Contributor Author

I think this PR is bettered targeted at main branch. We can backport it when we have something that we are confident in. We can merge experimental or partial impls to main.

@He-Pin Brought up an interesting point in that this may only be needed in 1.0.x because there is a presumption that once you migrate from an Akka cluster to a Pekko one running 1.0.x, there is no reason that the feature would be needed in 1.1.x (even though we are technically "breaking users").

Given this and the fact that there is a community agreement that all migration (from Akka) related features should land in Pekko 1.0.x I think leaving this to target 1.0.x is the right decision.

@pjfanning
Copy link
Contributor

I disagree about having this as a v1.0.x only feature. We've already had users who have tried to switch to Pekko from a version of Akka newer than v2.6. In the real world, we will have Akka users who reach the size where they need to pay license fees. If we add this feature, there is no reason not to keep it for a while. And if we then agree that it is useful to support this for a while (maybe 2 years as an example) then that makes it tidier to develop this on main branch - especially since we can release milestone versions.

@mdedetrich
Copy link
Contributor Author

I disagree about having this as a v1.0.x only feature. We've already had users who have tried to switch to Pekko from a version of Akka newer than v2.6. In the real world, we will have Akka users who reach the size where they need to pay license fees. If we add this feature, there is no reason not to keep it for a while. And if we then agree that it is useful to support this for a while (maybe 2 years as an example) then that makes it tidier to develop this on main branch - especially since we can release milestone versions.

I'm not disagreeing about it being in both versions, I am just saying that it has to land in 1.0.x as agreed upon and if it's going in 1.1.x as well then it makes sense for the feature to be complete and tested before the 1.1.0 full release otherwise we will have feature gaps

@pjfanning
Copy link
Contributor

I want to see this in 1.1.0-M1. It makes it easier for people to try it out. M1 is also a release that we can caveat. A 1.0.3 release would be expected to have well tested, working code. This change still feels experimental but worth experimenting with nonetheless.

@pjfanning
Copy link
Contributor

@mdedetrich noone seems to object to a 1.0.3-M1. Can we merge this? If we end up with an urgent 1.0.x bug fix issue, we could revert this again - but the likelihood is that the next 1.0.x release will be 1.0.3-M1. It would be nice to have these changes in a snapshot for some pre 1.0.3-M1 testing.

@mdedetrich mdedetrich marked this pull request as ready for review January 26, 2024 11:01
@mdedetrich
Copy link
Contributor Author

@mdedetrich noone seems to object to a 1.0.3-M1. Can we merge this? If we end up with an urgent 1.0.x bug fix issue, we could revert this again - but the likelihood is that the next 1.0.x release will be 1.0.3-M1. It would be nice to have these changes in a snapshot for some pre 1.0.3-M1 testing.

Sure, changed it to review status. It just needs a review.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@mdedetrich mdedetrich merged commit 4b78adf into apache:1.0.x Jan 26, 2024
18 checks passed
@mdedetrich mdedetrich deleted the handle-mixed-akka-pekko-protocol-names branch January 26, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants