Enable deserialization of old Akka cluster messages (mixed pekko/akka cluster) #1578

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

Jump to bottom

Open

sadekmunawar wants to merge 1 commit into apache:main

from sadekmunawar:akka-deserialization

Open

Enable deserialization of old Akka cluster messages (mixed pekko/akka cluster) #1578

sadekmunawar wants to merge 1 commit into apache:main from sadekmunawar:akka-deserialization

Conversation

@sadekmunawar

Copy link

Contributor

@sadekmunawar sadekmunawar commented Dec 10, 2024

Forming a cluster with Akka nodes requires the deserialization of cluster messages sent by the Akka. This commit fixes the following exception that occurs during deserialization.

[akka://HybridCluster@127.0.0.1:2551] with serializer id [5] and manifest [akka.cluster.InternalClusterAction$InitJoinAck].
java.lang.IllegalArgumentException: Unknown manifest [akka.cluster.InternalClusterAction$InitJoinAck]
 at org.apache.pekko.cluster.protobuf.ClusterMessageSerializer.fromBinary(ClusterMessageSerializer.scala:156)


 Enable deserialization of old Akka cluster messages

a54c5e9

@sadekmunawar sadekmunawar mentioned this pull request

Dec 10, 2024

Allow cluster formation with mixed protocols #1567

Merged

He-Pin

He-Pin reviewed

Dec 10, 2024

View reviewed changes

cluster/src/main/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializer.scala

case _ => throw new IllegalArgumentException(s"Unknown manifest [$manifest]")

def fromBinary(bytes: Array[Byte], manifest: String): AnyRef = {

val updatedManifest = {

if (manifest.startsWith("akka"))

Copy link

Member

Choose a reason for hiding this comment

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

this is only needed when migrating from Akka, may be better under a boolean guard.

Copy link

Member

Choose a reason for hiding this comment

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

IIRC, @mdedetrich once added one, and then is can be if (guard && manifest.startsWith("akka")), WDYT

Copy link

Member

@pjfanning pjfanning Dec 10, 2024

Choose a reason for hiding this comment

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

I can provide a PR for this based on checking pekko.remote.accept-protocol-names config. That config is an array value and if "akka" is in the array then we can allow this check. We only need to do this config once so the boolean result can be stored as a val.

Copy link

Contributor Author

@sadekmunawar sadekmunawar Dec 11, 2024

Choose a reason for hiding this comment

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

I agree. If we decide to keep the changes in this PR, then having boolean guard would be better.

pjfanning

pjfanning reviewed

Dec 10, 2024

View reviewed changes

cluster/src/test/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializerSpec.scala

}

"be de-serializable with class manifests from Akka nodes" in {

val oldAkkaJoinAckManifest = s"org.apache.pekko.cluster.InternalClusterAction$$InitJoinAck"

Copy link

Member

@pjfanning pjfanning Dec 10, 2024

Choose a reason for hiding this comment

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

@sadekmunawar this class only needs to be supported for Akka prior to v2.6.4.

Do we really need to support clusters that are running with very old Akka releases? Ideally we would only support pretty recent Akka releases. It is a pity that Akka changed the cluster messages in a patch release (2.6.5).

pekko/cluster/src/main/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializer.scala

Lines 44 to 58 in 5d4e828

// Kept for one version iteration from Akka 2.6.4 to allow rolling migration to short manifests

// can be removed in Akka 2.6.6 or later.

val OldJoinManifest = s"org.apache.pekko.cluster.InternalClusterAction$$Join"

val OldWelcomeManifest = s"org.apache.pekko.cluster.InternalClusterAction$$Welcome"

val OldLeaveManifest = s"org.apache.pekko.cluster.ClusterUserAction$$Leave"

val OldDownManifest = s"org.apache.pekko.cluster.ClusterUserAction$$Down"

val OldInitJoinManifest = s"org.apache.pekko.cluster.InternalClusterAction$$InitJoin$$"

val OldInitJoinAckManifest = s"org.apache.pekko.cluster.InternalClusterAction$$InitJoinAck"

val OldInitJoinNackManifest = s"org.apache.pekko.cluster.InternalClusterAction$$InitJoinNack"

val HeartBeatManifestPre2523 = s"org.apache.pekko.cluster.ClusterHeartbeatSender$$Heartbeat"

val HeartBeatRspManifest2523 = s"org.apache.pekko.cluster.ClusterHeartbeatSender$$HeartbeatRsp"

val OldExitingConfirmedManifest = s"org.apache.pekko.cluster.InternalClusterAction$$ExitingConfirmed"

val OldGossipStatusManifest = "org.apache.pekko.cluster.GossipStatus"

val OldGossipEnvelopeManifest = "org.apache.pekko.cluster.GossipEnvelope"

val OldClusterRouterPoolManifest = "org.apache.pekko.cluster.routing.ClusterRouterPool"

@pjfanning

Copy link

Member

pjfanning commented Dec 11, 2024

I have updated https://cwiki.apache.org/confluence/display/PEKKO/Pekko+Akka+Compatibility and include the fact the we only support forming clusters with Akka nodes of version 2.6.5 and above.

@raboof

Copy link

Member

raboof commented Jan 10, 2025

Does that mean we can close this PR?

@pjfanning

Copy link

Member

pjfanning commented Jan 10, 2025

I would prefer not to use this because I think trying to support Akka before v2.6.5 almost certainly will lead to us having to add extra fixes. One that I suspect that we'd need is that we might need to also allow Pekko nodes to optionally send cluster messages that look like the old Akka format and this PR does not address that.

@raboof

Copy link

Member

raboof commented Jan 10, 2025

I would prefer not to use this because I think trying to support Akka before v2.6.5 almost certainly will lead to us having to add extra fixes.

Sounds reasonable to me

Labels

None yet

4 participants

@sadekmunawar @pjfanning @raboof @He-Pin

Enable deserialization of old Akka cluster messages (mixed pekko/akka cluster) #1578

Are you sure you want to change the base?

Enable deserialization of old Akka cluster messages (mixed pekko/akka cluster) #1578

Uh oh!

Conversation

@sadekmunawar sadekmunawar commented Dec 10, 2024

Uh oh!

@He-Pin He-Pin Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

@He-Pin He-Pin Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

@pjfanning pjfanning Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

@sadekmunawar sadekmunawar Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

@pjfanning pjfanning Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

pjfanning commented Dec 11, 2024

Uh oh!

raboof commented Jan 10, 2025

Uh oh!

pjfanning commented Jan 10, 2025

Uh oh!

raboof commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants