Updated PersistenceMessageSerializer #3069

alexvaluyskiy · 2017-09-05T07:00:32Z

alexvaluyskiy · 2017-09-05T07:01:18Z

src/core/Akka.Persistence.Tests/Serialization/MessageSerializerRemotingSpec.cs

-        public void MessageSerializer_should_serialize_state_change_event()
-        {
-            var p1 = new Persistent(new PersistentFSM.StateChangeEvent("a", TimeSpan.FromSeconds(10)), sender: TestActor);
-            var serializer = _serialization.FindSerializerFor(p1);


We should not test all our serialization infrastructure here, just a concrete serializer.

You moved this and the test above to a seperate spec file. But the tests remain the same. So what do you mean ?

I stopped using FindSerializerFor and added a test for AtLeastOnceDeliverySnapshot
We have integrations tests for all standard persistence messages in Persistence.TCK

alexvaluyskiy · 2017-09-05T07:02:07Z

src/core/Akka.Persistence.Tests/Serialization/MessageSerializerRemotingSpec.cs

-        public void MessageSerializer_should_serialize_fsm_snapshot()
-        {
-            var snapshot = new PersistentFSM.PersistentFSMSnapshot<MyPayload>("a", new MyPayload("b"), TimeSpan.FromSeconds(10));
-            var p1 = new Persistent(snapshot, sender: TestActor);


PersistentFSMSnapshot should not be put into Persistent object

Correct. There is a special case scenario in the PersistenceMessageSerializer to handle it. Putting it in the Persistent envelope is wrong.

alexvaluyskiy · 2017-09-05T07:02:35Z

src/core/Akka.Persistence/Serialization/PersistenceMessageSerializer.cs

+            if (obj is AtomicWrite aw) return GetAtomicWrite(aw).ToByteArray();
+            if (obj is AtLeastOnceDeliverySnapshot snap) return GetAtLeastOnceDeliverySnapshot(snap).ToByteArray();
+            if (obj is PersistentFSM.StateChangeEvent stateEvent) return GetStateChangeEvent(stateEvent).ToByteArray();
+            if (obj.GetType().GetTypeInfo().IsGenericType 


obj.GetType().GetTypeInfo().IsGenericType - it prevents us from unhandled exceptions

It will still cause an exception down the line. Namely the ArgumentException. But that will be for more descriptive.

ArgumentException is fine, by design

Danthar · 2017-09-05T09:16:36Z

src/core/Akka.Persistence/Serialization/PersistenceMessageSerializer.cs

                payload.PayloadManifest = ByteString.CopyFromUtf8(manifest);
            }
            else
            {
                if (serializer.IncludeManifest)
                {
-                    var payloadType = obj.GetType();
-                    payload.PayloadManifest = ByteString.CopyFromUtf8(payloadType.AssemblyQualifiedName);
+                    payload.PayloadManifest = ByteString.CopyFromUtf8(obj.GetType().TypeQualifiedName());


Type qualified name is without the assemblyname right ?

It looks like this is an extension method in the Akka.Util package, which will attempt to serialize without the assembly name if it is part of the core assembly. I'm a little bit nervous on how well this works across runtime versions and if there are going to be an issues during upgrades. The change can be found at d3328ca

Yeah, so I just did a quick comparison. A mono 4.8 app has a core assembly name of mscorlib, while a .net core standard app has a core assembly name of System.Private.CoreLib. This could cause issues with people that are trying to switch runtimes. I'll create an issue.

@joshgarnett is this an issue with this specific change, or also with the previous code?

Actually I take that back, in theory the default should always deserialize properly for types with no assembly included?

@heynickc Anyway, we have used the similar method in 1.2.x

Yeah, I think we are fine, I retracted my comment right after yours @heynickc I was concerned at first, but all this is doing is grouping together objects that would be under the core assembly, which should be the same set of objects regardless of the actual name of the core assembly.

@alexvaluyskiy ok sorry, I was in the wrong vacinity. this is where I was curious about such an issue: https://github.com/akkadotnet/akka.net/blob/dev/src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs#L656

@heynickc as for IncludeManifest, that is how serializers should work, right? If they do not set IncludeManifest to true, then the assumption is that the serializer does not need the manifest in order to deserialize the data properly.

@joshgarnett gotcha, thanks. I ran into the issue testing backwards compatibility, but I may have been testing an edge case that wasn't possible.

joshgarnett · 2017-09-05T12:27:16Z

src/core/Akka.Persistence/Serialization/PersistenceMessageSerializer.cs

+            if (obj is AtLeastOnceDeliverySnapshot snap) return GetAtLeastOnceDeliverySnapshot(snap).ToByteArray();
+            if (obj is PersistentFSM.StateChangeEvent stateEvent) return GetStateChangeEvent(stateEvent).ToByteArray();
+            if (obj.GetType().GetTypeInfo().IsGenericType 
+                && obj.GetType().GetGenericTypeDefinition() == typeof(PersistentFSM.PersistentFSMSnapshot<>)) return GetPersistentFSMSnapshot(obj).ToByteArray();

            throw new ArgumentException($"Can't serialize object of type [{obj.GetType()}] in [{GetType()}]");


Should this also be updated to SerializationException?

I sync it with JVM version. They have NonSerializableException in FromBinary and they don't treat this exception as a fatal

Updated PersistenceMessageSerializer

f2deac9

alexvaluyskiy commented Sep 5, 2017

View reviewed changes

fix api

a3a6d2a

Danthar reviewed Sep 5, 2017

View reviewed changes

joshgarnett reviewed Sep 5, 2017

View reviewed changes

Aaronontheweb and others added 2 commits September 5, 2017 10:03

Merge branch 'dev' into fixpersistenceserializer

9ff3ee3

Merge branch 'dev' into fixpersistenceserializer

e38bb9e

heynickc merged commit 2acaa55 into akkadotnet:dev Sep 5, 2017

alexvaluyskiy deleted the fixpersistenceserializer branch September 5, 2017 17:47

Updated PersistenceMessageSerializer #3069

Updated PersistenceMessageSerializer #3069

Uh oh!

Conversation

alexvaluyskiy commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexvaluyskiy commented Sep 5, 2017 •

edited

Loading