Fix Cluster Sharding private replicator recovery - #8480
Conversation
Watch role-scoped sharding DData replicators and recreate them at their stable actor paths when they terminate. Use actor selections so existing coordinators and remember-entities stores continue communicating with the replacement incarnation. Add regression coverage for DData remember entities and the persistence compatibility configuration.
|
|
||
| public class ClusterShardingReplicatorResiliencySpec : AkkaSpec | ||
| { | ||
| private sealed record EntityEnvelope(string EntityId); |
There was a problem hiding this comment.
why not use ShardEnvelope and the HashCodeMessageExtractor.Create here, no?
| } | ||
|
|
||
| private static Config SpecConfig => | ||
| ConfigurationFactory.ParseString(@" |
There was a problem hiding this comment.
LGTM - this is the config we want to test
| } | ||
|
|
||
| [Fact] | ||
| public async Task Private_replicator_should_recover_at_the_same_path_without_restarting_consumers() |
There was a problem hiding this comment.
Backwards compat test - make sure that the ActorSelection paths have not diverged between deployments, otherwise the deployment + shard rebalancing it triggers is going to be unable to reconcile until the new version is completely cycled in.
| shardWatcher.Watch(await Sys.ActorSelection(shard).ResolveOne(3.Seconds())); | ||
|
|
||
| var firstReplicator = await Sys.ActorSelection(replicatorPath).ResolveOne(3.Seconds()); | ||
| Watch(firstReplicator); |
| var coordinatorWatcher = CreateTestProbe(); | ||
| coordinatorWatcher.Watch(coordinator); | ||
| var shardWatcher = CreateTestProbe(); | ||
| shardWatcher.Watch(await Sys.ActorSelection(shard).ResolveOne(3.Seconds())); |
There was a problem hiding this comment.
WatchAsync for all of these calls
| _replicatorSettingsByRole = _replicatorSettingsByRole.SetItem(role, replicatorSettings); | ||
| } | ||
|
|
||
| return Context.ActorSelection(Self.Path / name); |
There was a problem hiding this comment.
this sucks - we should just stick with the IActorRef and return that rather than an ICanTell + ActorSelection
|
|
||
| private IActorRef CreateReplicator(string role, ReplicatorSettings settings) | ||
| { | ||
| var replicator = Context.Watch(Context.ActorOf( |
There was a problem hiding this comment.
Why not use Context.WatchWith and a custom message type so we know exactly which replicator + role died when we receive the termination message? That would avoid some of the messy look-ups inside TryHandleReplicatorTermination
| private bool TryHandleReplicatorTermination(IActorRef terminated) | ||
| { | ||
| string role = null; | ||
| foreach (var (candidateRole, replicator) in _replicatorsByRole) |
There was a problem hiding this comment.
see previous comments about using Context.WatchWith to avoid the lookups here
| if (_clusterShuttingDown || _cluster.IsTerminated) | ||
| return true; | ||
|
|
||
| _log.Warning( |
There was a problem hiding this comment.
Log an error, not a warning.
|
|
||
| Receive<Terminated>(msg => | ||
| { | ||
| if (TryHandleReplicatorTermination(msg.ActorRef)) |
There was a problem hiding this comment.
One thing we might need here is a flag for when we're shutting down ourselves, so we don't log a bunch of errors when the shard region is being terminated as part of a normal node exit.
Keep IActorRef throughout the sharding DData consumers and publish a local replacement message when the guardian recreates a terminated replicator. Existing coordinators and remember-entities stores swap to the new ref and retry their active operation, while providers use the replacement for newly-created stores. Use WatchWith role-aware termination messages, preserve the peer-visible actor path, suppress shutdown recreation, and simplify the regression test fixtures.
|
Addressed the review in 1ca3718:
The focused recovery tests passed four consecutive rebuilt runs. The full sharding suite was 193/194 with only the unchanged RememberEntitiesStarterSpec timing failure noted in the PR description. |
Aaronontheweb
left a comment
There was a problem hiding this comment.
LGTM - I refined this and tried some alternative approaches with the BackoffSupervisor but I think this is probably the best option / least disruptive for Akka.Cluster.Sharding.
| public ClusterShardingSettings Settings { get; } | ||
| public int MajorityMinCap { get; } | ||
| public IActorRef Replicator { get; } | ||
| public ICanTell Replicator { get; } |
There was a problem hiding this comment.
Moving to ICanTell allows us to communicate with the replicator based on its ActorPath via ActorSelection, so in the event that it dies / gets replaced this will continue to work - unlike an IActorRef which will get invalidated.
I would've gone with the IActorRef-based approach but it requires A LOT more accounting and moving parts, so I settled for this.
* Fix Cluster Sharding private replicator recovery Watch role-scoped sharding DData replicators and recreate them at their stable actor paths when they terminate. Use actor selections so existing coordinators and remember-entities stores continue communicating with the replacement incarnation. Add regression coverage for DData remember entities and the persistence compatibility configuration. * Address sharding replicator recovery review Keep IActorRef throughout the sharding DData consumers and publish a local replacement message when the guardian recreates a terminated replicator. Existing coordinators and remember-entities stores swap to the new ref and retry their active operation, while providers use the replacement for newly-created stores. Use WatchWith role-aware termination messages, preserve the peer-visible actor path, suppress shutdown recreation, and simplify the regression test fixtures. * Simplify remember entities provider tracking * Simplify sharding replicator recovery * Cover sharding replicator coordinated shutdown
Summary
ICanTellactor selection for the replicator's stable path, so existing coordinators and remember-entities stores automatically address the replacement actor.Fixes #8471
Compatibility and scope
/system/sharding/<role>Replicatorpath, preserving compatibility with older cluster members.remember-entities-store = ddatais configured.INoSerializationVerificationNeeded.Akka.DistributedData.Validation
Akka.Cluster.Sharding.Testsnet10.0 build: 0 warnings and 0 errors.