Persisence serializer setting #2933

Danthar · 2017-08-04T12:35:27Z

Work in progress for the Persistence serializer proposal. See #2743 (comment)

Danthar · 2017-08-04T12:36:43Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Settings.cs

@@ -130,6 +135,7 @@ public SnapshotStoreSettings(Config config)
            SchemaName = config.GetString("schema-name");
            TableName = config.GetString("table-name");
            AutoInitialize = config.GetBoolean("auto-initialize");
+            DefaultSerializer = config.GetString("serializer");


Even if this setting is not specified by the plugin explicitly, it should still be available. See changes in persistence.conf

Danthar · 2017-08-04T12:38:40Z

src/core/Akka.Persistence/persistence.conf

@@ -115,6 +115,9 @@ akka.persistence {
        # Dispatcher for message replay.
        replay-dispatcher = "akka.persistence.dispatchers.default-replay-dispatcher"

+        # Default serializer used as manifest serializer when applicable and payload serializer when no specific binding overrides are specified
+        serializer = "json"
+


Here i defined the default serializer setting in the journal-plugin-fallback. Which should be used by default as the fallback plugin for every journal plugin, by the persistence subsystem. But I still need to verify this is actually true

Danthar · 2017-08-04T12:39:05Z

src/core/Akka.Persistence/persistence.conf

@@ -171,6 +174,9 @@ akka.persistence {
        # Dispatcher for the plugin actor.
        plugin-dispatcher = "akka.persistence.dispatchers.default-plugin-dispatcher"

+        # Default serializer used as manifest serializer when applicable and payload serializer when no specific binding overrides are specified
+        serializer = "json"
+


Same goes for this one, only then defined in the snapshot-store-plugin-fallback

Danthar · 2017-08-04T12:41:32Z

src/core/Akka/Serialization/Serialization.cs

@@ -119,6 +119,26 @@ public Serialization(ExtendedActorSystem system)
            }
        }

+        private Serializer GetSerializerByName(string name)
+        {


So this is the helper method for translating the serializer name to an actual instance.
Downside, having to translate it here. Upside, being able to leverage caching once this is done.

Danthar · 2017-08-04T12:42:27Z

src/core/Akka/Serialization/Serialization.cs

+                return null;
+
+            var serializersConfig = System.Settings.Config.GetConfig("akka.actor.serializers").AsEnumerable().ToList();
+            foreach (var kvp in serializersConfig)


Because this config is not saved in the Serialization subsystem, I have to retrieve this value again. Leaning on the assumption of hocon config caching here.

Danthar · 2017-08-04T12:46:05Z

src/core/Akka/Serialization/Serialization.cs

+                    var serializerType = Type.GetType(serializerTypeName);
+
+                    var serializerId = SerializerIdentifierHelper.GetSerializerIdentifierFromConfig(serializerType, (ExtendedActorSystem)System);
+                    return GetSerializerById(serializerId);


Because I want to leverage existing infrastructure as much as possible, without having to resort to creating a new map from name to id (which is possibly easier, but costs more memory, although small). I resolve the name to the serializer ID and use the existing map to return the instance based on that ID.

Im still not completely set on this implementation though. I might just go with a seperate dict that maps name -> id which is initialised in the constructor like the other mappings are.

Impact of this method should be small though, as its only called once per type, after which caching takes over.

Impact of this method should be small though, as its only called once per type, after which caching takes over.

IMHO, should be fine.

Danthar · 2017-08-05T11:31:23Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs

@@ -507,7 +515,7 @@ public RequestChunk(int chunkId, IJournalRequest[] requests)
        private readonly HashSet<IActorRef> _allIdsSubscribers;
        private readonly HashSet<string> _allPersistenceIds;

-        private readonly Func<Type, Serializer> _getSerializer;
+        private readonly Func<Type,string, Serializer> _getSerializer;


@Horusiath why did you used this pattern? Why not simply call Context.System.Serialization.FindSerializerFor When where you need it? Why store a reference in a func ?

Danthar · 2017-08-07T13:33:28Z

Needs ApiApproval. But im going to hold off with that untill i get some feedback.

Danthar · 2017-08-08T14:29:35Z

Ready for review. Should probably update the DData integration as well. Which should not be hard to do. But want to get some feedback first.

heynickc · 2017-08-09T15:36:39Z

@Danthar can you add a quick spec or two for the key parts of this?

Aaronontheweb

Some minor changes needed, namely some unhappy path spec coverage.

Aaronontheweb · 2017-08-10T19:51:36Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs

@@ -224,6 +224,11 @@ public abstract class BatchingSqlJournalSetup
        public QueryConfiguration NamingConventions { get; }

        /// <summary>
+        /// The default serializer used when not type override matching is found
+        /// </summary>
+        public readonly string DefaultSerializer;


Make this a property

Aaronontheweb · 2017-08-10T19:51:53Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs

@@ -163,6 +163,11 @@ public class QueryConfiguration
        public readonly TimeSpan Timeout;

        /// <summary>
+        /// The default serializer used when not type override matching is found
+        /// </summary>
+        public readonly string DefaultSerializer;


Same as above

Aaronontheweb · 2017-08-10T20:15:23Z

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

@@ -209,5 +209,14 @@ public void MessageSerializer_should_serialize_manifest_provided_by_EventAdapter

            back.Manifest.ShouldBe(p1.Manifest);
        }
+
+        [Fact]
+        public void Serialization_respects_default_serializer_parameter()


Add a spec to demonstrate what happens if the default serializer can't be found

Aaronontheweb · 2017-08-10T20:16:45Z

src/core/Akka/Serialization/Serialization.cs

+                    var serializerType = Type.GetType(serializerTypeName);
+
+                    var serializerId = SerializerIdentifierHelper.GetSerializerIdentifierFromConfig(serializerType, (ExtendedActorSystem)System);
+                    return GetSerializerById(serializerId);


Impact of this method should be small though, as its only called once per type, after which caching takes over.

IMHO, should be fine.

Danthar · 2017-08-11T09:29:50Z

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

    old-payload = ""Akka.Persistence.Tests.Serialization.OldPayloadSerializer, Akka.Persistence.Tests""
+    testserializer = ""Akka.Serialization.HyperionSerializer, Akka.Serialization.Hyperion""


In order to fix these tests. I switched system default serializer for these tests to Hyperion.
But im not sure if thats a good idea.

In fact. After talking this over with @alexvaluyskiy we are not sure why we have this Spec at all. It was disabled before. But it does not test any real-world scenario as far as we know.
What are the real-world scenario's for sending the Persistent or AtomicWrite types across the network.

None that I can think of

Danthar · 2017-08-11T09:30:46Z

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

@@ -40,7 +40,7 @@ public void Serialization_respects_default_serializer_parameter()
        {
            var message = new TestMessage("this is my test message");
            var serializer = _serialization.FindSerializerFor(message, "json");
-            Assert.True(serializer.Identifier == 1); //by default configuration the serializer id for json == newtonsoft == 1
+            Assert.Equal(1, serializer.Identifier); //by default configuration the serializer id for json == newtonsoft == 1


Upside is that this test is more explicit.

Danthar · 2017-08-11T09:30:52Z

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

@@ -50,7 +50,7 @@ public void Serialization_falls_back_to_system_default_if_unknown_serializer_par
            var serializer = _serialization.FindSerializerFor(message, "unicorn");
            //since unicorn is an unknown serializer, the system default will be used
            //which incedentally is JSON at the moment
-            Assert.True(serializer.Identifier == 1); //by default configuration the serializer id for json == newtonsoft == 1
+            Assert.Equal(-5, serializer.Identifier); //system serializer is currently configured to be hyperion which is -5


Same goes for this one

…comments

- Updated LocalSnapshotStore - Updated persistence.conf

Aaronontheweb

Looks fine, sans a copyright header that needs to be changed

Aaronontheweb · 2017-08-11T16:40:20Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs

@@ -224,6 +224,11 @@ public abstract class BatchingSqlJournalSetup
        public QueryConfiguration NamingConventions { get; }

        /// <summary>
+        /// The default serializer used when not type override matching is found
+        /// </summary>
+        public string DefaultSerializer { get; }


Looks good to me

Aaronontheweb · 2017-08-11T17:20:20Z

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

    old-payload = ""Akka.Persistence.Tests.Serialization.OldPayloadSerializer, Akka.Persistence.Tests""
+    testserializer = ""Akka.Serialization.HyperionSerializer, Akka.Serialization.Hyperion""


None that I can think of

Aaronontheweb · 2017-08-11T17:20:49Z

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

@@ -0,0 +1,56 @@
+// <copyright file="SerializationSpec.cs" company="Copaco B.V.">
+//        Copyright (c) 2015 - 2017 All Right Reserved
+//        Author: Arjen Smits


Copyright header needs to be changed

@Danthar indicated in Gitter chat that this was a mistake with his ReSharper template, so no worries.

Danthar commented Aug 4, 2017

View reviewed changes

Danthar commented Aug 5, 2017

View reviewed changes

Danthar added serialization needs review labels Aug 5, 2017

Danthar changed the title ~~WIP Persisence serializer setting~~ Persisence serializer setting Aug 8, 2017

Danthar added this to the 1.3.0 milestone Aug 9, 2017

Aaronontheweb requested changes Aug 10, 2017

View reviewed changes

Danthar commented Aug 11, 2017

View reviewed changes

Danthar and others added 11 commits August 11, 2017 16:21

Renamed files to reflect their single contained type. And added some …

ff7dbff

…comments

Added default config setting for plugin and snapshot serializer for json

ef19d00

Updated Serialization API to account new scenario

dc4517b

- Updated LocalSnapshotStore - Updated persistence.conf

Updated SQL Integrations to reflect changes

c3e241a

Adjust author info

c17d4f9

Api approval

387f564

Added spec for testing fallback scenario

7ba3b1d

Fixed fallback scenario

b9df564

Fixed filenames and fixes in MessageSerializerRemotingSpec

ca22a79

Fixed MessageSerializerRemoting

9045816

Processed remarks

ef5780c

Aaronontheweb requested changes Aug 11, 2017

View reviewed changes

Aaronontheweb added 2 commits August 11, 2017 13:06

changed copyright header

d71bf77

Merge branch 'v1.3' into persistence-setting

99a5565

Aaronontheweb approved these changes Aug 11, 2017

View reviewed changes

Aaronontheweb merged commit eee1eba into akkadotnet:v1.3 Aug 11, 2017

Aaronontheweb mentioned this pull request Jul 12, 2019

Redo Akka.Persistence serialization #3811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persisence serializer setting #2933

Persisence serializer setting #2933

Danthar commented Aug 4, 2017

Danthar Aug 4, 2017

Danthar Aug 4, 2017

Danthar Aug 4, 2017

Danthar Aug 4, 2017

Danthar Aug 4, 2017

Danthar Aug 4, 2017

Aaronontheweb Aug 10, 2017

Danthar Aug 5, 2017

Danthar commented Aug 7, 2017

Danthar commented Aug 8, 2017

heynickc commented Aug 9, 2017

Aaronontheweb left a comment

Aaronontheweb Aug 10, 2017

Aaronontheweb Aug 10, 2017

Aaronontheweb Aug 10, 2017

Aaronontheweb Aug 10, 2017

Danthar Aug 11, 2017

Danthar Aug 11, 2017 •

edited

Loading

Aaronontheweb Aug 11, 2017

Danthar Aug 11, 2017

Danthar Aug 11, 2017

Aaronontheweb left a comment

Aaronontheweb Aug 11, 2017

Aaronontheweb Aug 11, 2017

Aaronontheweb Aug 11, 2017

Aaronontheweb Aug 11, 2017

		old-payload = ""Akka.Persistence.Tests.Serialization.OldPayloadSerializer, Akka.Persistence.Tests""
		testserializer = ""Akka.Serialization.HyperionSerializer, Akka.Serialization.Hyperion""

Persisence serializer setting #2933

Persisence serializer setting #2933

Conversation

Danthar commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danthar commented Aug 7, 2017

Danthar commented Aug 8, 2017

heynickc commented Aug 9, 2017

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danthar Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danthar Aug 11, 2017 •

edited

Loading