Repository navigation
Conversation
… another session Signed-off-by: Dream95 <zhou_8621@163.com>
…close Signed-off-by: Dream95 <zhou_8621@163.com>
lhotari
left a comment
There was a problem hiding this comment.
LGTM. Thanks for adding the version check.
I traced isCreatedBySelf() for each store. ZooKeeper compares the ephemeral owner with the client's current session id (ZKMetadataStore.java:516-519), Oxia compares its stable client identifier (OxiaMetadataStore.java:144-151), and Memory/RocksDB always report true. After a session loss the election re-creates its node under the new session on SessionReestablished (LeaderElectionImpl.java:394-397), so a legitimate leader still deletes its own node on close. Tolerating NotFound is also an improvement: previously, closing after the node had vanished failed the close.
I reverted the main change locally and closeOfStaleLeaderAfterFollowerTakeoverDoesNotDeleteTheNewLeadersNode fails without the fix and passes with it. The two inline points are non-blocking.
A possible follow-up, not for this PR: a test in pulsar-metadata against real ZooKeeper (BaseMetadataStoreTest already has TestZKServer, and ZKSessionTest.java:160-235 expires a real session with zks.expireSession(...)) where several participants repeatedly lose their session and close, asserting that a close never removes another session's node and that exactly one participant eventually leads. Oxia could reuse it once there is a way to expire a single client's session there.
| if (t instanceof NotFoundException || t instanceof BadVersionException) { | ||
| // The node is no longer the one we read: it was deleted, or re-created | ||
| // by another participant with a higher version, between our get() and | ||
| // this delete(). |
There was a problem hiding this comment.
[QUALITY] the version check does not protect against re-creation on ZooKeeper and the in-memory store, and the comment says it does
Nit: the comment says the node may have been "re-created by another participant with a higher version". On ZooKeeper (and LocalMemory/MockZooKeeper) a re-created node starts at version 0 again, so if this instance read its own node at version 0 and the other participant deletes and re-creates it before the delete runs, delete(path, Optional.of(0)) succeeds against the new node and neither NotFound nor BadVersion is raised. Oxia's versions are shard-wide, so there a re-created key does get a new version.
The PR description already says the fix is not atomic on ZooKeeper, which is fine; I'd just reword this comment (and keep the description) so it doesn't suggest the version check closes the window. The version still helps for the case where the leader node is updated in place.
| assertEquals(le1.getState(), LeaderElectionState.NoLeader); | ||
| // le2 kept the leadership: exactly the initial Following and the takeover Leading, with no | ||
| // extra loss/regain cycle triggered by le1's close. | ||
| assertEquals(le2States, List.of(LeaderElectionState.Following, LeaderElectionState.Leading)); |
There was a problem hiding this comment.
[QUALITY] neither test pins the version argument or the BadVersion handling
I confirmed that this test fails when asyncClose() is reverted to the old unconditional delete, so the main scenario is covered. Two small gaps:
- The foreign node is created before
le1.close()runs, so close stops at theisCreatedBySelf()check and never reaches the versioned delete. ChangingOptional.of(version)back toOptional.empty()after the ownership check, or dropping theBadVersionExceptionbranch, would still pass both tests. A deterministic variant is possible withFaultInjectionMetadataStore(main source set): overridedeleteso it first updates the leader node from another store and then delegates the delete with the original expected version, and assert that close succeeds and the node survives. - In the failing run without the fix, the
survivorassertion on line 133 did not fire by itself:le2sees theDeletednotification and immediately re-creates the node, so the node exists again by the time it is read. What actually pins the fix is thele2Statesassertion on line 139 (an extraNoLeader/Leadingcycle). That is fine, but a short comment saying so would help the next reader.
| .thenAccept(__ -> { | ||
| synchronized (LeaderElectionImpl.this) { | ||
| leaderElectionState = LeaderElectionState.NoLeader; | ||
| // The deleted leader node was ours and a closed instance no longer | ||
| // observes elections; don't keep reporting ourselves as leader. | ||
| // Whether or not we deleted the node, a closed instance no longer observes | ||
| // elections; don't keep reporting ourselves as leader. | ||
| currentLeaderFuture = CompletableFuture.completedFuture(Optional.empty()); | ||
| } |
There was a problem hiding this comment.
One lifecycle edge case worth checking: elect() supports reopening an instance after close, but the old asyncClose() could still be waiting on this get().
If elect() runs before that close future completes, the old close might delete the newly re-elected leader node, and this callback could reset its state back to NoLeader.
I think the normal close-then-start path is fine since it waits for close to finish. Could we clarify whether overlapping asyncClose() and elect() is supported? If it is, we’d probably need to guard this completion against a newer election cycle.
Not blocking this fix, just something worth checking.
… LeaderElection close Signed-off-by: Dream95 <zhou_8621@163.com>
Motivation
When a
LeaderElectioninstance is closed in theLeadingstate,asyncClose()deletes the leader node at its path. That delete is unconditional: theversionfield passed tostore.delete()is never assigned and always holdsOptional.empty().If the store session expires while the instance is leading, the ephemeral leader node is removed on the server side, and the stale instance never learns about the deletion. Another participant then wins the re-election and creates its own node at the same path. Closing the stale instance deletes that node, so the new leader loses leadership and has to run a new election for no reason.
Modifications
asyncClose()now reads the leader node first and deletes it only if it exists andstat.isCreatedBySelf()is true; otherwise the node is left alone. ANotFoundExceptionbetween the read and the delete is ignored.versionfield.On ZooKeeper ,the read and delete are separate store calls, so the fix isn't atomic. Still, it narrows the race from the stale instance's remaining lifetime to a few milliseconds between them, which is good enough in practice.
Verifying this change
This change added tests and can be verified as follows:
LeaderElectionStaleLeaderCloseTest.closeOfStaleLeaderAfterFollowerTakeoverDoesNotDeleteTheNewLeadersNodeLeaderElectionStaleLeaderCloseTest.closeOfActualLeaderStillDeletesItsOwnNodeRun with:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes