Allow PersistentShardCoordinator to tolerate duplicate ShardHomeAllocated messages
#5967
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
…located` messages close akkadotnet#5604
Aaronontheweb
changed the title
PersistentShardCoordinator to tolerate duplicate `ShardHomeAl…PersistentShardCoordinator to tolerate duplicate ShardHomeAllocated messages
Aaronontheweb
commented
May 26, 2022
| /// itself and go offline.</param> | ||
| /// <exception cref="ArgumentException">Thrown if an event is illegal in the current state.</exception> | ||
| /// <returns>An update copy of this state.</returns> | ||
| public State Updated(IDomainEvent e, bool isRecovering = false) |
There was a problem hiding this comment.
We now accept a new parameter here, isRecovering - when this value is set to true then we enable "reader tolerance" inside the PersistentShardCoordinator.
| @@ -153,7 +156,27 @@ public State Updated(IDomainEvent e) | |||
| if (!Regions.TryGetValue(message.Region, out var shardRegions)) | |||
| throw new ArgumentException($"Region {message.Region} not registered", nameof(e)); | |||
| if (Shards.ContainsKey(message.Shard)) | |||
| throw new ArgumentException($"Shard {message.Shard} is already allocated", nameof(e)); | |||
| { | |||
| if (!isRecovering) | |||
There was a problem hiding this comment.
I've decided to implement reader tolerance in only one location for now the ShardHomeAllocated message, since this is where the issue has come up most often and it's also probably the safest place to do it.
| // we're going to allow new value to overwrite previous | ||
| var newRegions = Regions; | ||
| var previousRegion = Shards[message.Shard]; | ||
| if (Regions.TryGetValue(previousRegion, out var previousShards)) |
There was a problem hiding this comment.
ALGO is as follows:
- Take previous
ShardRegionactor, if any, removeshardfrom its collection. - Add shard to new
ShardRegionactor. Addshardto its collection. - It is very likely that both
ShardRegionactors are the same in many cases, in which these two operations are identical to what the normal happy path does - but in case they are not, results in the newShardHomeAllocatedmessage overwriting the old one.
| @@ -1346,26 +1369,26 @@ protected override bool ReceiveRecover(object message) | |||
| switch (evt) | |||
| { | |||
| case ShardRegionRegistered _: | |||
There was a problem hiding this comment.
Make sure all of the Recovery methods are updated to support the new isRecovering flag.
Arkatufus
added a commit
to Arkatufus/akka.net
that referenced
this pull request
May 26, 2022
…erate duplicate ShardHomeAllocated messages
|
Conversation on how safe these changes are #5970 (comment) |
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
May 30, 2022
…rdHomeAllocated` messages (akkadotnet#5967)" This reverts commit 4fa7abb.
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
May 31, 2022
…rdHomeAllocated` messages (akkadotnet#5967)" This reverts commit 4fa7abb.
4 tasks
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.
Fixes #5604
Changes
Allows the
PersistentShardCoordinatorto tolerate multipleShardHomeAllocatedmessages in the journal for the same shard - this is done in order to allow the sharding system to try its best to restart itself without bringing the system offline in the event of duplicate data appearing in the journal.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
PersistentShardCoordinatora tolerant reader #5604