Fix sharding recovery error and WithTransport serialization #3744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o par with JVM
Based on the equivalent JVM spec
|
Got massively interrupted (airport) while I was working on this, so it's not in a compile-able state. Will prioritize this ASAP though. Think there's some possibilities for Akka.Remote performance improvements too. |
…o C# that we haven't quite done
…uentially This was done in order to avoid the two specs trying to bind on the same port at the same time.
…d snapshot stores
Aaronontheweb
commented
May 25, 2019
| @@ -42,5 +42,8 @@ class = ""Akka.Persistence.Sqlite.Journal.BatchingSqliteJournal, Akka.Persistenc | |||
| } | |||
| }"); | |||
| } | |||
|
|
|||
| // TODO: hack. Replace when https://github.com/akkadotnet/akka.net/issues/3811 | |||
| protected override bool SupportsSerialization => false; | |||
There was a problem hiding this comment.
Another reason for #3811 - none of the Akka.Persistence.Sql journals support WriterGuid at the moment due to their hand-rolled serialization layers, so the serialization specs will not pass.
This was referenced Jun 6, 2019
Merged
This was referenced Jun 26, 2019
|
This PR is finished - complete port of akka/akka#25068 sans the Artery bits. |
Horusiath
requested changes
Jul 10, 2019
| string manifest = ""; | ||
| if (serializer is SerializerWithStringManifest) | ||
| // TODO: hack. Replace when https://github.com/akkadotnet/akka.net/issues/3811 | ||
| Akka.Serialization.Serialization.WithTransport(_serialization.System, () => |
There was a problem hiding this comment.
Am I right that WithTransport sets a specific ambient context for serialization data? If so maybe it would be easier to just pass that context directly into inner function parameter? How application will behave when we'll have multiple actor systems living in the same process, serializing at the same time?
There was a problem hiding this comment.
We don't pass it in because other libraries may not directly have access to when it's set via Akka.Remote - rather it gets parked ambiently in the local thread's storage and then unset when it's done being used. Won't be an issue for multiple actor systems due to the latter - the reference always gets unset and not leaked from the thread. There's more comments on the JVM issue that I ported this from: akka/akka#25068
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="SqlSnapshotStore"/> class. | ||
| /// </summary> | ||
| /// <param name="config">The configuration used to configure the snapshot store.</param> | ||
| protected SqlSnapshotStore(Config config) | ||
| { | ||
| _actorSystem = Context.System.AsInstanceOf<ExtendedActorSystem>(); |
There was a problem hiding this comment.
What is the advantage of AsInstanceOf over direct type cast? Honestly in my opinion, this is part which follows Scala over simple logic.
| { | ||
| if (serializer.IncludeManifest) | ||
| { | ||
| payload.PayloadManifest = ByteString.CopyFromUtf8(obj.GetType().TypeQualifiedName()); |
There was a problem hiding this comment.
Have you checked how TypeQualifiedName works for mscorlib between .NET Core and Full Framework? IIRC, there was an issue there since two frameworks used different naming for standard lib assembly.
There was a problem hiding this comment.
This is the same method we called before - I just moved it into a different place.
There was a problem hiding this comment.
I.e. I kept it consistent with its previous behavior as of 1.3.13
…/Aaronontheweb/akka.net into fix-3414-sharding-recovery-error
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Jul 21, 2019
…et#3744) * fixed typo in RemoteActorRefProvider comment * Working on akkadotnet#3414 - bringing SerializeWithTransport API up to par with JVM * added spec to help validate CurrentTransportInformation issues Based on the equivalent JVM spec * working on bringing serialization up to snuff * brought serialization class up to snuff * wrapping up RmeoteActorRefProvider implementation * WIP * cleaning up Serialization class * looks like there's a Lazy<SerializationInfo> translation from Scala to C# that we haven't quite done * fixed Serialization class * fixed bug with Akka.Remote.Serialization.SerializationTransportInformationSpec * forced a couple of specs using default akka.remote configs to run sequentially This was done in order to avoid the two specs trying to bind on the same port at the same time. * added serialization verification to the Akka.Persistence.TCK * fixed issues with default Akka.Perisstence.TCK specs * fixed IActorRef serialziation support in Akka.Persistence journals and snapshot stores * fixed compilation issuyes * fixed Akka.Sql.Common serialization in a backwards-compatible fashion * had to disable serialization specs for Sql Journals * Added API approvals * updated creator and serialize-all-messages serialization * added ITestOutputHelper to Akka.Cluster.Sharding.Tests.SupervisionSpec * made changes to LocalSnapshotSerializer * fixed bug in WithTransport method * updated Akka.Remote MessageSerializer
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Jul 21, 2019
…et#3744) * fixed typo in RemoteActorRefProvider comment * Working on akkadotnet#3414 - bringing SerializeWithTransport API up to par with JVM * added spec to help validate CurrentTransportInformation issues Based on the equivalent JVM spec * working on bringing serialization up to snuff * brought serialization class up to snuff * wrapping up RmeoteActorRefProvider implementation * WIP * cleaning up Serialization class * looks like there's a Lazy<SerializationInfo> translation from Scala to C# that we haven't quite done * fixed Serialization class * fixed bug with Akka.Remote.Serialization.SerializationTransportInformationSpec * forced a couple of specs using default akka.remote configs to run sequentially This was done in order to avoid the two specs trying to bind on the same port at the same time. * added serialization verification to the Akka.Persistence.TCK * fixed issues with default Akka.Perisstence.TCK specs * fixed IActorRef serialziation support in Akka.Persistence journals and snapshot stores * fixed compilation issuyes * fixed Akka.Sql.Common serialization in a backwards-compatible fashion * had to disable serialization specs for Sql Journals * Added API approvals * updated creator and serialize-all-messages serialization * added ITestOutputHelper to Akka.Cluster.Sharding.Tests.SupervisionSpec * made changes to LocalSnapshotSerializer * fixed bug in WithTransport method * updated Akka.Remote MessageSerializer
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Jul 26, 2019
…et#3744) * fixed typo in RemoteActorRefProvider comment * Working on akkadotnet#3414 - bringing SerializeWithTransport API up to par with JVM * added spec to help validate CurrentTransportInformation issues Based on the equivalent JVM spec * working on bringing serialization up to snuff * brought serialization class up to snuff * wrapping up RmeoteActorRefProvider implementation * WIP * cleaning up Serialization class * looks like there's a Lazy<SerializationInfo> translation from Scala to C# that we haven't quite done * fixed Serialization class * fixed bug with Akka.Remote.Serialization.SerializationTransportInformationSpec * forced a couple of specs using default akka.remote configs to run sequentially This was done in order to avoid the two specs trying to bind on the same port at the same time. * added serialization verification to the Akka.Persistence.TCK * fixed issues with default Akka.Perisstence.TCK specs * fixed IActorRef serialziation support in Akka.Persistence journals and snapshot stores * fixed compilation issuyes * fixed Akka.Sql.Common serialization in a backwards-compatible fashion * had to disable serialization specs for Sql Journals * Added API approvals * updated creator and serialize-all-messages serialization * added ITestOutputHelper to Akka.Cluster.Sharding.Tests.SupervisionSpec * made changes to LocalSnapshotSerializer * fixed bug in WithTransport method * updated Akka.Remote MessageSerializer
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Jul 30, 2019
…et#3744) * fixed typo in RemoteActorRefProvider comment * Working on akkadotnet#3414 - bringing SerializeWithTransport API up to par with JVM * added spec to help validate CurrentTransportInformation issues Based on the equivalent JVM spec * working on bringing serialization up to snuff * brought serialization class up to snuff * wrapping up RmeoteActorRefProvider implementation * WIP * cleaning up Serialization class * looks like there's a Lazy<SerializationInfo> translation from Scala to C# that we haven't quite done * fixed Serialization class * fixed bug with Akka.Remote.Serialization.SerializationTransportInformationSpec * forced a couple of specs using default akka.remote configs to run sequentially This was done in order to avoid the two specs trying to bind on the same port at the same time. * added serialization verification to the Akka.Persistence.TCK * fixed issues with default Akka.Perisstence.TCK specs * fixed IActorRef serialziation support in Akka.Persistence journals and snapshot stores * fixed compilation issuyes * fixed Akka.Sql.Common serialization in a backwards-compatible fashion * had to disable serialization specs for Sql Journals * Added API approvals * updated creator and serialize-all-messages serialization * added ITestOutputHelper to Akka.Cluster.Sharding.Tests.SupervisionSpec * made changes to LocalSnapshotSerializer * fixed bug in WithTransport method * updated Akka.Remote MessageSerializer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should resolve #3414 and related issues.