Repository navigation
DistributedData: Replicator accepts writes from a member it first saw as Leaving or Exiting - #8547
Aaronontheweb wants to merge 2 commits into
Conversation
1448a6c to
ecab3fb
Compare
… as Leaving or Exiting Replicator subscribes to cluster events with InitialStateAsEvents, which replays every current member at its CURRENT status. A replicator that starts after a member has already moved to Leaving or Exiting therefore receives MemberLeft/MemberExited for it and never MemberUp, so the member never lands in _nodes, and IsKnownNode silently drops every Write and gossip message that member sends for as long as it stays in the cluster. In cluster sharding this can stall a departing shard coordinator's ddata write for the whole updating-state-timeout during a rolling restart or coordinated shutdown, because the coordinator singleton hand-off cannot proceed until that write completes or times out. IsKnownNode now also accepts a node the replicator has seen in any member event (new _seenNodes set, pruned on MemberRemoved) or that is in the existing _exitingNodes set. No quorum or majority calculation reads either set, so read and write quorum sizing is unchanged. Adds ReplicatorKnownNodeSpec with three cases: a Write from a member first seen as Leaving is accepted, one first seen as Exiting is accepted, and one from a node never seen in any member event is still rejected.
… fact; name it distinctly Each fact now owns its peer ActorSystem and shuts it down with ShutdownAsync in a finally block, so teardown no longer pins a thread pool thread in the synchronous AfterAll and no DisposeAsync is declared on the class, which would collide with the async dispose chain the TestKit gains in #8545. The peer system gets a distinct name; it self-joins its own cluster and Sys never joins it, so nothing depends on the two names matching.
ecab3fb to
fb8bc40
Compare
|
Opened #8582 as an alternative implementation of the same fix, for comparison. The approach is the same — stop dropping
private bool IsKnownNode(Address node) => _nodes.Contains(node) || _weaklyUpNodes.Contains(node) ||
_joiningNodes.Contains(node) || _exitingNodes.Contains(node) ||
_leader.Any(x => x.Address == node) || _selfAddress == node;That's a 2-line production change versus a new field with new populate/cleanup sites. I also confirmed the Verified against the spec from this PR, with one addition. Running the three facts here unchanged against the alternative:
I added a fourth fact for Full suite on #8582: 194 passed, 1 skipped, 0 failed. One note relevant to both PRs: Apache Pekko retains the original four-term |
|
Closing in favor of #8582, which fixes the same gate by reusing the sets the Replicator already maintains instead of adding a new one, and adds the Downed case to the spec. |
What changes
Replicator.IsKnownNodenow also accepts a member the replicator has seen in any membership state and a member in its exiting set. Before, it accepted only members it had seen reachUp,WeaklyUp, orJoining, plus itself.Why
The replicator subscribes to cluster events with
InitialStateAsEvents, which replays each member at its current status. A replicator that starts after a member has already reachedLeavingreceivesMemberLeftand neverMemberUp. OnlyMemberUpadded a member to the known set, so everyWrite,Read, andDeltaPropagationthat member sent was dropped as an unknown node for as long as it stayed in the cluster. A delete is aWriteof a tombstone, so deletes were dropped too. Gossip and status messages were not affected: they are gated on the destination system uid and never reach this check.Cluster sharding hits this during a coordinated shutdown. When the oldest node leaves, the departing shard coordinator writes its final state through the replicator on the next-oldest node. If that replicator started while the oldest node was already leaving, it drops the write, and the coordinator burns its full 5 s
updating-state-timeoutbefore the singleton hands over. That was 5.0 of the 5.6 secondsClusterShardingRegistrationCoordinatedShutdownSpecwaited on the Artery lane, where it failed four times in two days. Pekko has the same gap. The test-side fix for that spec is #8544; this PR removes the waste itself.Quorum and majority sizing are untouched.
IsKnownNodehas one call site, the inbound gate, and every majority reads the up, exiting, and age-ordered sets directly. This is a bug fix that restores intended behavior, so it is not recorded in the 1.6 breaking-changes ledger.How it was checked
Akka.DistributedDatabuilds with warnings as errors.Akka.DistributedData.Tests: 193 passed, 1 skipped as before.Akka.API.Tests: 18 passed, no approval change; the new state is private. The newReplicatorKnownNodeSpechas three cases: a write from a member first seen asLeavingis accepted, a write from a member first seen asExitingis accepted, and a write from an address never seen in any member event is still rejected. The first two fail on the old code with the exact "Ignoring message [Write] ... unknown node" log line.Second commit: the spec's teardown
From the adversarial review: the spec shut its peer system down with a blocking call in
AfterAll, and named it the same as the test system. Each fact now shuts the peer down withShutdownAsyncin afinallyblock, so teardown runs on success or failure without pinning a thread. The class deliberately declares noDisposeAsync, because the TestKit gains an async dispose chain in #8545 and a bare method of that name on a derived class would then hide it and fail the build with warnings as errors. The peer is named with a-peersuffix; it self-joins its own cluster and the test system never joins it, so nothing depends on the names matching. Run three times: 3 passed each time.