Copilot commented on code in PR #3659:
URL: https://github.com/apache/celeborn/pull/3659#discussion_r3179140450
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:
##########
@@ -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;
Review Comment:
`stepDown()` no longer logs on success. Since
`StateMachine.notifyLogFailed(...)` calls `masterRatisServer.stepDown()` and
ignores the returned boolean, a successful demotion becomes silent and only
failures are visible. Consider logging an INFO on success inside `stepDown()`
(or updating call sites to log based on the boolean result).
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:
##########
@@ -274,9 +276,36 @@ public void start() throws IOException {
}
public void stop() {
+ stop(false);
+ }
+
+ public void stop(boolean transferLeadership) {
+ if (!stopped.compareAndSet(false, true)) {
+ LOG.info("Raft server {} already stopped.", server.getId());
+ return;
+ }
try {
+ if (transferLeadership && isLeader()) {
+ LOG.info(
+ "This node {} is the Raft leader. Transferring leadership before
shutdown.",
+ server.getId());
+ long startTime = System.currentTimeMillis();
+ boolean success = stepDown();
+ long elapsed = System.currentTimeMillis() - startTime;
+ if (success) {
+ LOG.info("Successfully transferred leadership from {} in {}ms.",
server.getId(), elapsed);
+ } else {
+ LOG.warn(
+ "Leadership transfer from {} failed after {}ms. "
+ + "Proceeding with shutdown anyway.",
+ server.getId(),
+ elapsed);
+ }
+ }
server.close();
+ LOG.info("Raft server {} closed.", server.getId());
} catch (IOException e) {
Review Comment:
`stop(boolean)` marks the server as stopped (`stopped.compareAndSet(false,
true)`) before attempting leadership transfer and `server.close()`. If
`server.close()` throws, subsequent stop attempts will be skipped and the
server may remain partially running. Consider only setting `stopped` after a
successful close, or resetting it when stop fails.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -2761,6 +2762,17 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
+ val HA_MASTER_GRACEFUL_SHUTDOWN_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.master.ha.gracefulShutdown.enabled")
+ .categories("ha")
+ .version("0.7.0")
+ .doc("When true, the master will transfer Raft leadership " +
+ "before shutting down gracefully. This reduces chances of " +
+ "client side failures by avoiding the Raft election window " +
+ "where no leader is available.")
+ .booleanConf
+ .createWithDefault(false)
Review Comment:
The new config key uses camelCase
(`celeborn.master.ha.gracefulShutdown.enabled`), while existing Celeborn
configs use dot-separated lowercase words (e.g.
`celeborn.worker.graceful.shutdown.enabled`,
`celeborn.worker.graceful.shutdown.timeout`). Since this is a new user-facing
key, consider switching to `celeborn.master.ha.graceful.shutdown.enabled` (and
optionally keeping the current spelling as an `.withAlternative(...)` for
compatibility) to match established naming conventions.
##########
master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java:
##########
@@ -1872,6 +1872,56 @@ public void testReviseShuffles() throws
InterruptedException {
Assert.assertEquals(STATUSSYSTEM3.registeredShuffleCount(), 8);
}
+ @Test
+ public void testGracefulLeaderShutdownStepDown() {
+ // Identify the current leader
+ HARaftServer leader = null;
+ for (HARaftServer server : Arrays.asList(RATISSERVER1, RATISSERVER2,
RATISSERVER3)) {
+ if (server.isLeader()) {
+ leader = server;
+ break;
+ }
+ }
+ Assert.assertNotNull("A leader should exist before the test", leader);
+ Assert.assertTrue("Leader node should report isLeader=true",
leader.isLeader());
+
+ // Do not stop() the shared static server used by the suite, since that
permanently
+ // closes one of RATISSERVER1/2/3 and can break later tests depending on
execution order.
+ // Instead, trigger the leader to step down without closing the underlying
server.
+ leader
+ .getMasterStateMachine()
+ .notifyLogFailed(new Exception("test leader graceful step down"),
null);
+
+ Assert.assertFalse(
+ "Leader should step down without closing the shared server",
leader.isLeader());
Review Comment:
`testGracefulLeaderShutdownStepDown` asserts `leader.isLeader()` becomes
false immediately after `notifyLogFailed(...)`. However
`HARaftServer.isLeader()` returns `true` early when `cachedPeerRole` is already
LEADER, and `stepDown()` does not update the cache. This makes the assertion
timing-dependent/flaky. Consider explicitly refreshing role state (e.g., call
`leader.updateServerRole()` or poll until `isLeader()` flips) before asserting.
##########
master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java:
##########
@@ -1872,6 +1872,56 @@ public void testReviseShuffles() throws
InterruptedException {
Assert.assertEquals(STATUSSYSTEM3.registeredShuffleCount(), 8);
}
+ @Test
+ public void testGracefulLeaderShutdownStepDown() {
+ // Identify the current leader
+ HARaftServer leader = null;
+ for (HARaftServer server : Arrays.asList(RATISSERVER1, RATISSERVER2,
RATISSERVER3)) {
+ if (server.isLeader()) {
+ leader = server;
+ break;
+ }
+ }
+ Assert.assertNotNull("A leader should exist before the test", leader);
+ Assert.assertTrue("Leader node should report isLeader=true",
leader.isLeader());
+
+ // Do not stop() the shared static server used by the suite, since that
permanently
+ // closes one of RATISSERVER1/2/3 and can break later tests depending on
execution order.
+ // Instead, trigger the leader to step down without closing the underlying
server.
+ leader
+ .getMasterStateMachine()
+ .notifyLogFailed(new Exception("test leader graceful step down"),
null);
+
Review Comment:
This test is named around “graceful leader shutdown”, but it triggers
step-down via `StateMachine.notifyLogFailed(...)` (a log-failure callback)
rather than exercising the new shutdown path (`HARaftServer.stop(true)` /
`Master.stop` + `ha.gracefulShutdown` config). As written it doesn't validate
the new behavior introduced by this PR; consider either renaming the test to
reflect what it's actually verifying, or adding an assertion that directly
covers the shutdown/transfer-leadership flow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
