[CELEBORN-2306] Master adds shutdown hook for ratis stepdown by kaybhutani · Pull Request #3659 · apache/celeborn

kaybhutani · 2026-04-12T21:34:28Z

What changes were proposed in this pull request?

Adds master shutdown hook
Updates RAFT stepdown to return bool
Calls RAFT stepdown on manager shutdown to do graceful stepdown

Why are the changes needed?

Manager.stop() isnt being called from anywhere, it emmits some logs as well for "Stopping manager" but since there is no shutdown hook defined, none of them are logged or the function is called at all
We faced a certain issue where the leader got removed from service mesh before shutdown and followers redirected to it because it was still running (able to send requests but not receive) for a brief period. This method adds a graceful shutdown option to do a RATIS stepdown before shutting down.

Does this PR resolve a correctness bug?

No.

Does this PR introduce any user-facing change?

Yes, adds an additional config.

How was this patch tested?

Added a tests and validated that.

Copilot

Pull request overview

This PR adds a Master JVM shutdown hook and introduces an HA “graceful shutdown” path intended to transfer Raft leadership before shutdown, reducing client impact during leader termination.

Changes:

Add a Master shutdown hook to invoke Master.stop(...) on JVM termination.
Add HA graceful shutdown config and, when enabled, stop the Ratis server during Master shutdown (including leader transfer).
Update HARaftServer.stepDown() to return a boolean and add a system test around leader shutdown behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java	Adds a new HA test that stops the leader Raft server and asserts closure.
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala	Registers a JVM shutdown hook and conditionally stops the Raft server in `onStop()`.
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java	Enhances `stop()` to attempt leadership transfer; changes `stepDown()` to return success/failure.
docs/configuration/ha.md	Documents the new HA graceful shutdown config flag.
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala	Adds the new config entry and accessor for HA graceful shutdown enablement.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

SteNicholas · 2026-04-20T03:32:53Z

⏺ Review: [CELEBORN-2306] Master shutdown hook and Raft stepdown

PR: #3659 | Author: kaybhutani | Change: +93 / -2 across 5 files

What it does

Adds a JVM shutdown hook to the Celeborn Master that, when celeborn.master.ha.gracefulShutdown.enabled is set, transfers Raft leadership to another node before shutting down. This avoids the Raft election window where no leader is available
and clients see failures.

Key changes:

New config: celeborn.master.ha.gracefulShutdown.enabled (default false)
HARaftServer.stepDown() now returns boolean
HARaftServer.stop() attempts leadership transfer before closing
Shutdown hook registered in Master.initialize()
New test for leader step-down + close

Issues

(High) Shutdown hook may never trigger Raft transfer

The shutdown hook calls stop(EXIT_IMMEDIATELY) which calls rpcEnv.stop(self), scheduling onStop() asynchronously on the RPC dispatcher. Since dispatcher threads are daemon threads, the JVM can exit before onStop() runs — meaning the
leadership transfer never happens.

Fix: Perform the Raft step-down directly in Master.stop() or the shutdown hook itself, not in the async onStop() callback. Or use a CountDownLatch to block the hook until onStop() completes.

(High) Double-close of Raft server

When graceful shutdown is enabled and the node is leader: Master.onStop() calls ratisServer.stop(), then the normal statusSystem/HAMasterMetaManager shutdown path likely calls it again. This can throw IOException or produce confusing logs.
HARaftServer.stop() should be idempotent (e.g., guard with an AtomicBoolean).

(Medium) Config/doc mismatch

The shutdown hook is registered unconditionally in Master.initialize(), but the config description says "the master will run a shutdown hook." The config actually only gates the Raft transfer inside onStop(). Either gate the hook registration
on the config, or fix the description.

(Medium) Test corrupts shared cluster state

testGracefulLeaderShutdownStepDown calls leader.stop() on a shared static RATISSERVER instance. This permanently closes the leader, breaking any subsequent tests that assume a 3-node cluster. The test should either spin up an isolated
cluster, or restart the server in a finally block.

(Low) Leadership transfer always attempted in stop(), regardless of config

HARaftServer.stop() unconditionally attempts transfer when the node is leader. The config gate in Master.onStop() only controls whether stop() is called early. This is arguably fine (best-effort), but worth documenting.

(Low) No independent timeout on transfer in stop()

The TransferLeadershipRequest has a 60s timeout, but stop() itself has no guard. If the Ratis call hangs, shutdown stalls. Consider wrapping in a Future with a bounded timeout.

(Nit) Duplicate success logging

A successful transfer now logs from both stepDown() ("Successfully step down leader") and stop() ("Successfully transferred leadership"). Remove one.

Verdict

Request changes. The motivation is solid — graceful leadership transfer is valuable for production HA. But the critical issue is that the Raft transfer logic in onStop() may never execute due to daemon thread timing (Issue 1), and
double-close needs a guard (Issue 2). The test also needs isolation (Issue 4).

Suggested fix priority:

Move Raft shutdown into Master.stop() / the hook directly, not onStop()
Make HARaftServer.stop() idempotent
Fix config description
Isolate the test

SteNicholas · 2026-04-20T03:33:28Z

@kaybhutani, thanks for contribution. Please address above comments.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

SteNicholas · 2026-04-22T03:59:41Z

      }
    } catch (Exception e) {
      LOG.warn("Step down leader failed!", e);
+      return false;


We could only return false at the end of this method.

SteNicholas · 2026-04-22T04:05:22Z

@kaybhutani, thanks for updates. Please run UPDATE=1 build/mvn clean test -pl common -am -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuite to add document for applying suggestion of celeborn.master.ha.gracefulShutdown.enabled.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kaybhutani · 2026-04-23T19:59:01Z

made changes @SteNicholas

SteNicholas · 2026-04-30T04:51:50Z

@kaybhutani, thanks for update. Please fix the failure of CI that GrpcRatisMasterStatusSystemSuiteJ>RatisMasterStatusSystemSuiteJ.testGracefulLeaderShutdownStepDown:1919 A new leader should be elected after step down.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kaybhutani · 2026-05-04T07:02:01Z

@@ -628,13 +657,13 @@ void stepDown() {
              REQUEST_TIMEOUT_MS);
      RaftClientReply reply = server.transferLeadership(request);
      if (reply.isSuccess()) {
-        LOG.info("Successfully step down leader {}.", server.getId());
-      } else {
-        LOG.warn("Step down leader failed!");
+        return true;
      }
+      LOG.warn("Step down leader {} failed.", server.getId());
    } catch (Exception e) {
-      LOG.warn("Step down leader failed!", e);
+      LOG.warn("Step down leader {} failed.", server.getId(), e);
    }
+    return false;


We already log Successfully transferred leadership from {} in {}ms. on successful transfer, this one is redundant as per me but open to add it if reviewers disagree.

kaybhutani added 2 commits April 13, 2026 02:45

raft leadership transfer

f787bf3

fix

1c3dcd9

github-actions Bot added kind:documentation module:common module:master labels Apr 12, 2026

kaybhutani changed the title ~~Master should call shutdown hooks and transfer raft leadership gracefully~~ [CELEBORN-2306] Master should call shutdown hooks and transfer raft leadership gracefully Apr 12, 2026

kaybhutani changed the title ~~[CELEBORN-2306] Master should call shutdown hooks and transfer raft leadership gracefully~~ [CELEBORN-2306] Register Master shutdown and transfer Raft leadership gracefully Apr 12, 2026

kaybhutani changed the title ~~[CELEBORN-2306] Register Master shutdown and transfer Raft leadership gracefully~~ [CELEBORN-2306] Register Master shutdown hooks and transfer Raft leadership gracefully Apr 12, 2026

kaybhutani changed the title ~~[CELEBORN-2306] Register Master shutdown hooks and transfer Raft leadership gracefully~~ [CELEBORN-2306] Master shutdown hook and Raft stepdown Apr 12, 2026

kaybhutani marked this pull request as draft April 12, 2026 21:40

kaybhutani closed this Apr 12, 2026

fix

b86116c

kaybhutani reopened this Apr 17, 2026

kaybhutani marked this pull request as ready for review April 17, 2026 07:04

SteNicholas requested a review from Copilot April 20, 2026 03:00

Copilot started reviewing on behalf of SteNicholas April 20, 2026 03:00 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

Apply suggestions from code review

e3b108c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

SteNicholas requested a review from zaynt4606 April 22, 2026 03:58

SteNicholas reviewed Apr 22, 2026

View reviewed changes

SteNicholas requested a review from Copilot April 22, 2026 04:02

Copilot started reviewing on behalf of SteNicholas April 22, 2026 04:02 View session

SteNicholas changed the title ~~[CELEBORN-2306] Master shutdown hook and Raft stepdown~~ [CELEBORN-2306] Master adds shutdown hook for ratis stepdown Apr 22, 2026

Copilot AI reviewed Apr 22, 2026

View reviewed changes

fix

860538f

fix

b085fa3

SteNicholas requested a review from Copilot May 4, 2026 02:06

Copilot started reviewing on behalf of SteNicholas May 4, 2026 02:07 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

kaybhutani added 2 commits May 4, 2026 12:28

fix

ec1142c

fix

cc46bdf

kaybhutani marked this pull request as draft May 4, 2026 10:50

kaybhutani added 2 commits May 4, 2026 16:30

fix

617d0c6

fix

006058b

kaybhutani marked this pull request as ready for review May 4, 2026 15:03

kaybhutani requested a review from SteNicholas May 4, 2026 15:03

Conversation

kaybhutani commented Apr 12, 2026 • edited by SteNicholas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR resolve a correctness bug?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SteNicholas commented Apr 20, 2026

Uh oh!

SteNicholas commented Apr 20, 2026

Uh oh!

SteNicholas Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

SteNicholas commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaybhutani commented Apr 23, 2026

Uh oh!

SteNicholas commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

kaybhutani May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaybhutani commented Apr 12, 2026 •

edited by SteNicholas

Loading