Enable deserialization of old Akka cluster messages (mixed pekko/akka cluster) #1578
Conversation
| case _ => throw new IllegalArgumentException(s"Unknown manifest [$manifest]") | ||
| def fromBinary(bytes: Array[Byte], manifest: String): AnyRef = { | ||
| val updatedManifest = { | ||
| if (manifest.startsWith("akka")) |
There was a problem hiding this comment.
this is only needed when migrating from Akka, may be better under a boolean guard.
There was a problem hiding this comment.
IIRC, @mdedetrich once added one, and then is can be if (guard && manifest.startsWith("akka")), WDYT
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree. If we decide to keep the changes in this PR, then having boolean guard would be better.
| @@ -177,6 +177,14 @@ class ClusterMessageSerializerSpec extends PekkoSpec("pekko.actor.provider = clu | |||
| ClusterMessageSerializer.OldWelcomeManifest) | |||
| } | |||
|
|
|||
| "be de-serializable with class manifests from Akka nodes" in { | |||
| val oldAkkaJoinAckManifest = s"org.apache.pekko.cluster.InternalClusterAction$$InitJoinAck" | |||
There was a problem hiding this comment.
@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).
|
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. |
|
Does that mean we can close this PR? |
|
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. |
Sounds reasonable to me |
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.