yew1eb commented on code in PR #3838:
URL: https://github.com/apache/celeborn/pull/3838#discussion_r3967855208
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:
##########
@@ -101,9 +101,16 @@ public class HARaftServer {
private final CelebornConf conf;
private final AtomicBoolean stopped = new AtomicBoolean(false);
+ private final AtomicBoolean unexpectedCloseHandled = new
AtomicBoolean(false);
private long workerTimeoutDeadline;
private long appTimeoutDeadline;
+ private Runnable unexpectedCloseHandler =
+ () -> {
+ LOG.error("Exiting master process because the raft server was closed
unexpectedly.");
+ System.exit(1);
+ };
+
Review Comment:
Fail-fast here is intentional: once the embedded RaftServer is closed by
Ratis (e.g. JvmPauseMonitor after a long pause), the master can no longer serve
any write and silently drops out of the quorum while keeping its RPC/HTTP ports
open. Exiting the process makes the failure explicit so external supervision
(systemd/K8s) can restart the node and let it rejoin — similar to how HDFS
NameNode terminates on fatal errors. The handler is already extracted as a
replaceable Runnable for testability, and users who prefer to keep the process
alive can set celeborn.master.ha.ratis.unexpected.close.action=none. The error
log already includes the detected lifecycle state as context.
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:
##########
@@ -311,6 +319,34 @@ public void stop(boolean transferLeadership) {
}
}
+ private void checkRaftServerState() {
+ if (stopped.get() || unexpectedCloseHandled.get()) {
+ return;
+ }
+ LifeCycle.State state = server.getLifeCycleState();
+ if ((state == LifeCycle.State.CLOSED || state == LifeCycle.State.EXCEPTION)
+ && unexpectedCloseHandled.compareAndSet(false, true)) {
+ LOG.error(
+ "Raft server {} was closed unexpectedly (state {}), e.g. by the
Ratis "
+ + "JvmPauseMonitor after a long JVM pause. Action: {}.",
+ server.getId(),
+ state,
+ conf.haMasterRatisUnexpectedCloseAction());
+ onUnexpectedRaftServerClose();
+ }
+ }
Review Comment:
Good catch — the description was broader than the implementation. We've
updated the PR description to match: the check triggers on terminal lifecycle
states (CLOSED / EXCEPTION). We intentionally do not broaden to state !=
RUNNING (transitional states like STARTING/CLOSING would misfire) nor add a
group-membership check, which would cost a getGroupInfo RPC on every role-check
interval.
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:
##########
@@ -540,6 +576,19 @@ public Optional<LeaderPeerEndpoints>
getCachedLeaderPeerRpcEndpoint() {
* Get the group info (peer role and leader peer id) from Ratis server and
update the server role.
*/
public void updateServerRole() {
+ LifeCycle.State state = server.getLifeCycleState();
+ if (state != LifeCycle.State.RUNNING) {
+ // Only log on role change to avoid flooding logs on every metrics
scrape.
+ if (cachedPeerRole.isPresent()) {
+ LOG.warn(
+ "Raft server {} is not RUNNING (state {}). Setting cached role to
UNRECOGNIZED "
+ + "and resetting leader info.",
+ server.getId(),
+ state);
+ }
+ setServerRole(null, null, null);
+ return;
+ }
Review Comment:
Fixed — the reset now only applies to terminal states (CLOSED / EXCEPTION);
transitional states keep the previous code path. Thanks!
##########
master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java:
##########
@@ -2145,4 +2147,31 @@ public void testRegisterApplicationInfo() {
assertEquals(statusSystem.applicationInfos.get(appId).userIdentifier(),
userIdentifier);
assertEquals(statusSystem.applicationInfos.get(appId).extraInfo(),
extraInfo);
}
+
+ @Test
+ public void testUnexpectedRaftServerClose() throws Exception {
+ HARaftServer nonLeader =
+ !RATISSERVER1.isLeader()
+ ? RATISSERVER1
+ : (!RATISSERVER2.isLeader() ? RATISSERVER2 : RATISSERVER3);
+ try {
+ AtomicBoolean handlerInvoked = new AtomicBoolean(false);
+ nonLeader.setUnexpectedCloseHandler(() -> handlerInvoked.set(true));
+
+ // Simulate the Ratis JvmPauseMonitor closing the local raft server.
+ nonLeader.getServer().close();
+ Assert.assertEquals(LifeCycle.State.CLOSED, nonLeader.getServerState());
+
+ long deadline = System.currentTimeMillis() + 30_000;
+ while (!handlerInvoked.get() && System.currentTimeMillis() < deadline) {
+ Thread.sleep(200L);
+ }
+ Assert.assertTrue(handlerInvoked.get());
Review Comment:
Fixed — checkRaftServerState() is now package-visible with
@VisibleForTesting, and the test invokes it once directly after closing the
server instead of polling the scheduler. Deterministic and much faster.
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:
##########
@@ -311,6 +319,34 @@ public void stop(boolean transferLeadership) {
}
}
+ private void checkRaftServerState() {
+ if (stopped.get() || unexpectedCloseHandled.get()) {
+ return;
+ }
+ LifeCycle.State state = server.getLifeCycleState();
+ if ((state == LifeCycle.State.CLOSED || state == LifeCycle.State.EXCEPTION)
+ && unexpectedCloseHandled.compareAndSet(false, true)) {
+ LOG.error(
+ "Raft server {} was closed unexpectedly (state {}), e.g. by the
Ratis "
+ + "JvmPauseMonitor after a long JVM pause. Action: {}.",
+ server.getId(),
+ state,
+ conf.haMasterRatisUnexpectedCloseAction());
+ onUnexpectedRaftServerClose();
+ }
+ }
+
+ void onUnexpectedRaftServerClose() {
+ if ("exit".equals(conf.haMasterRatisUnexpectedCloseAction())) {
+ unexpectedCloseHandler.run();
+ }
+ }
+
+ @VisibleForTesting
+ void setUnexpectedCloseHandler(Runnable handler) {
+ this.unexpectedCloseHandler = handler;
Review Comment:
Fixed — the setter now rejects null via Objects.requireNonNull.
--
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]
