[server] Wait for coordinator leader cleanup during shutdown#3649
Conversation
swuferhong
left a comment
swuferhong
left a comment
There was a problem hiding this comment.
LGTM overall. I left one comment.
| } | ||
|
|
||
| private void triggerCleanupLeaderServices() { | ||
| if (isLeader.compareAndSet(true, false)) { |
There was a problem hiding this comment.
isLeader is only set to true after initLeaderServices.run() completes. If close() is called before initialization completes, this problem also will be happen. The implementation should explicitly track initialization/closing state instead of using isLeader as both readiness state and cleanup ownership state?
There was a problem hiding this comment.
@loserwang1024 @swuferhong Thank you, left a suggestion, take a look if I'm missing smth, please.
Good catch @swuferhong 👍
Probably we can go even further:
There's also a case beyond close(): leadership flap while init is still running. notLeader's CAS sees false, cleanup is never queued, and the re-election starts a second init concurrently with the first, so pendingCleanup doesn't cover the exact case it was built for.
I checked why cleanup got its own thread in #2780 (and reasoning "avoid blocking init tasks" in the git history): both callbacks are synchronized in CoordinatorServer, so they already run one at a time and the extra thread never bought real parallelism. The only unlocked part is init's fence/register, and curator's bounded retries mean it can't hang forever.
So, combining: single-thread executor + a small state field written only from it STANDBY / INITIALIZING / LEADER / CLOSED.
Cleanup task cleans if INITIALIZING or LEADER (fenced init stops being special), isLeader() stays state == LEADER, all checks happen inside the tasks in event order, and pendingCleanup + the 60s wait get deleted.
It's the same shape as Flink's DefaultLeaderElectionService - single-thread leadershipOperationExecutor + explicit running state.
WDYT?
ce536a6 to
305571b
Compare
|
@fresh-borzoni It's exactly what I want to do! @fresh-borzoni @swuferhong , CC |
305571b to
e687f2e
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@loserwang1024 Thank you, looks good, only a few comments, PTAL
| if (initializationFailure == null && !closing.get()) { | ||
| state = State.LEADER; | ||
| } else { | ||
| cleanupLeaderServices(initializationFailure); |
There was a problem hiding this comment.
If init throws we clean up and go STANDBY but still hold the latch, so nothing re-elects and we're left with a leader that isn't serving.
Looks pre-existing, maybe a follow-up, but could we at least add a test for the cleanup path here? wdyt?
There was a problem hiding this comment.
You are right. This behavior comes from the existing strategy.
I also considered releasing the current leadership when leader initialization fails, for example by closing the current LeaderLatch. However, that alone would be problematic for a single-coordinator deployment, because there may be no other candidate to take over. To handle this correctly, we likely need to release the current election leadership and then rejoin the election, probably with some backoff to avoid a tight retry loop.
That recovery behavior is outside the scope of this PR, so I think we should handle it in a separate follow-up. For this PR, I added a regression/reproduction test to document the current behavior:
org.apache.fluss.server.coordinator.CoordinatorLeaderElectionTest#testInitializationFailureKeepsLeaderLatchAndBlocksReElection.
CoordinatorLeaderElection previously shut down its callback executor immediately after closing the LeaderLatch. Since the notLeader callback performs leader cleanup asynchronously on the same executor, shutdown could interrupt or cancel the cleanup task. Explicitly trigger the idempotent cleanup during close, wait for the pending cleanup to finish, and gracefully terminate the callback executor. Add a regression test verifying that close waits for leader cleanup.
e687f2e to
35376ec
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@loserwang1024 Thank you, LGTM 👍
3 participants
Purpose
Linked issue: close #3648
Brief change log
Tests
API and Format
Documentation