Fix serialize-messages for Akka.Cluster and Akka.Remote (netstandard2.0) #3791
Conversation
…ere missing from the 1.x library set). akkadotnet#3668
| /// <param name="upNumber">The upNumber of the member, as assigned by the leader at the time the node joined the cluster.</param> | ||
| /// <param name="status">The status of this member.</param> | ||
| /// <param name="roles">The roles for this member. Can be empty.</param> | ||
| [JsonConstructor] |
There was a problem hiding this comment.
isn't this serialized via protobuff ClusterMessageSerializer? Why json serialization?
There was a problem hiding this comment.
@zbynek001 A good question to ask. You're correct that there are a lot of protobuf encoded Cluster Messages (in ClusterMessages.g.cs), however this particular Member.cs class is actually being referenced in the messages defined under ClusterEvent.XYZ which are messages published to the EventBus (and in the context of the BugFix3724Spec test at least, are destined for the Event.LoggingBus).
I am new to the akka.net code base so take the following comments as very tentative:
- The messages under ClusterEvent.XYZ are JSON serialized by design at this point
- My gut says that protobuf would be better (even if we are waiting for Hyperion) but that would be a breaking change
- I'm keen to circle back to this comment in the context of netstandard2.0 to see whether Remove restrictions required by netstandard 1.x but available in 2.0 #3790 will enable the INoSerializationVerificationNeeded restriction to be lifted
@Aaronontheweb Do you have any input on this point?
There was a problem hiding this comment.
So after a quick tour through the code, I think that the underlying Gossip and state management messages for the cluster do use the protobuf messages, but the observable messages published by the ClusterDomainEventPublisher on the EventBus are JSON serialized.
I'm interested to know if this is a specific design decision and/or if there would be resistance to migrating to a consistent protobuf wire format for both?
There was a problem hiding this comment.
@zbynek001 @heatonmatthew so this might be an issue with serialize-messages inside Akka core where we don't use the correct serializer based on its manifest type.... I'm going to see if I can fix that in #3744 - I added a JsonConstructor call similar to this one for the Member class and that may no longer be necessary once the serialization fixes are added to serialize-messages in that PR.
There was a problem hiding this comment.
@Aaronontheweb let me know if there's anything specific you could do with a hand on. It's quite interesting code to be working on.
|
@heatonmatthew so I just merged in #3725 - but you have some additional code in this PR beyond my changes. What's the best way to reconcile this? |
|
@Aaronontheweb this one builds on my changes from #3790 which is waiting to be merged. The majority of the extra changes will be from that, so it would be helpful to progress that one first. I've just updated that branch now. Once #3790 is merged, I can review this against the combined set of changes. Since this whole branch is intended to be a cherry-pick of #3724 on top of #3790 (mainly so that I can use it in pre-production for myself), there might be nothing more to do with this specific task. |
|
Not needed - this is already fixed. |
Applied changes from #3725 on top of netstandard 2.0 targeted upgrade with the serialization restriction of dotnet core projects lifted (#3790).