-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix sharding recovery error and WithTransport serialization #3744
Changes from all commits
37660ed
531c7eb
0fc478f
160db40
0b84f3b
b7080e0
c30b68e
1d4c4f8
96238af
84de193
c336417
04fa972
d7eb903
7258288
b558492
5827048
9b02494
17ee26c
22075ca
98ed473
bd1df59
eb4597f
44cdd61
88ee076
69244be
632612e
f7ba0be
54cc1e6
94eaa40
6e944d5
cb43452
939aba3
cf2dbf4
4fd4e03
a4a0bf4
9814412
f9c6d86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
using Akka.Event; | ||
using Akka.Persistence.Snapshot; | ||
using Akka.Util; | ||
using Akka.Util.Internal; | ||
|
||
namespace Akka.Persistence.Sql.Common.Snapshot | ||
{ | ||
|
@@ -39,12 +40,15 @@ private Initialized() { } | |
|
||
private readonly SnapshotStoreSettings _settings; | ||
|
||
private readonly ExtendedActorSystem _actorSystem; | ||
|
||
/// <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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the advantage of |
||
_settings = new SnapshotStoreSettings(config); | ||
_pendingRequestsCancellation = new CancellationTokenSource(); | ||
} | ||
|
@@ -224,7 +228,9 @@ private SnapshotEntry ToSnapshotEntry(SnapshotMetadata metadata, object snapshot | |
var snapshotType = snapshot.GetType(); | ||
var serializer = Context.System.Serialization.FindSerializerForType(snapshotType, _settings.DefaultSerializer); | ||
|
||
var binary = serializer.ToBinary(snapshot); | ||
var binary = Akka.Serialization.Serialization.WithTransport(_actorSystem, | ||
() => serializer.ToBinary(snapshot)); | ||
|
||
|
||
return new SnapshotEntry( | ||
persistenceId: metadata.PersistenceId, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another reason for #3811 - none of the Akka.Persistence.Sql journals support |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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