Copilot commented on code in PR #10999:
URL: https://github.com/apache/ozone/pull/10999#discussion_r3764531918
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -426,6 +446,42 @@ private void tryStartDNServerAndRefreshSafeMode() {
}
}
+ /**
+ * Periodic fallback for the deferred datanode-server start on a follower.
The
+ * Ratis callbacks ({@code applyTransaction}, {@code notifyTermIndexUpdated},
+ * {@code notifyLeaderChanged}) only fire on new activity, so on an idle
+ * cluster a restarted follower can miss the moment the leader's committed
+ * index becomes observable (it is not yet known when {@code
notifyLeaderChanged}
+ * fires) and never start its datanode server, leaving it stuck in safe mode.
+ * This re-checks until the server starts, then cancels itself. Starting
stays
+ * gated by the same {@link #tryStartDNServerAndRefreshSafeMode()}
predicate, so
+ * it cannot start the server before genuine catch-up.
+ */
+ private void retryStartDNServerUntilReady() {
+ if (isStateMachineReady.get()) {
+ cancelDNServerStartRetry();
+ return;
+ }
+ try {
+ tryStartDNServerAndRefreshSafeMode();
+ } catch (Exception e) {
+ // Never let a transient failure (e.g. SCM not fully wired up yet during
+ // startup) kill the scheduled task; log and retry on the next tick.
+ LOG.warn("Retry of deferred datanode-server start failed; will retry",
e);
+ return;
+ }
+ if (isStateMachineReady.get()) {
+ cancelDNServerStartRetry();
+ }
+ }
+
+ private void cancelDNServerStartRetry() {
+ ScheduledFuture<?> future = dnServerStartRetryFuture;
+ if (future != null) {
+ future.cancel(false);
+ }
+ }
Review Comment:
`cancelDNServerStartRetry()` cancels the scheduled task but keeps
`dnServerStartRetryExecutor` (and its thread) alive for the remainder of the
SCM process lifetime. Since the intent is “self-cancels after startup”,
consider also shutting down the scheduled executor (or otherwise releasing its
thread) when the state machine becomes ready to avoid an extra long-lived
thread per SCM instance.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -569,6 +625,8 @@ public void close() throws IOException {
transactionBuffer.close();
HadoopExecutors.
shutdown(installSnapshotExecutor, LOG, 5, TimeUnit.SECONDS);
+ HadoopExecutors.
+ shutdown(dnServerStartRetryExecutor, LOG, 5, TimeUnit.SECONDS);
Review Comment:
During `close()`, the retry task can still be running or can start running
while shutdown/awaitTermination is in progress, which can race with teardown
and potentially extend close latency (up to the 5s wait) if the task is
mid-execution. Consider explicitly cancelling `dnServerStartRetryFuture` (and
ideally preventing further runs) before shutting down
`dnServerStartRetryExecutor`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java:
##########
@@ -82,6 +84,16 @@ public class SCMStateMachine extends BaseStateMachine {
private final boolean isInitialized;
private ExecutorService installSnapshotExecutor;
+ // Interval for the fallback retry that starts the deferred datanode-server
on
+ // a follower (see retryStartDNServerUntilReady).
+ private static final long DN_SERVER_START_RETRY_INTERVAL_MS = 1000L;
Review Comment:
The retry interval is a hard-coded constant. Since this affects operational
behavior (startup time vs. background work), consider making it configurable
(eg via SCM config) or expressing it as a `Duration`/time-based config key to
simplify tuning and reduce future code changes if the default needs adjustment.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
