platinumhamburg commented on code in PR #3651:
URL: https://github.com/apache/fluss/pull/3651#discussion_r3600179920
##########
fluss-server/src/main/java/org/apache/fluss/server/tablet/TabletServer.java:
##########
@@ -465,21 +478,8 @@ CompletableFuture<Void> stopServices() {
scannerManager.close();
}
- if (kvManager != null) {
- kvManager.shutdown();
- }
-
- if (remoteLogManager != null) {
- remoteLogManager.close();
- }
-
- if (logManager != null) {
- logManager.shutdown();
- }
-
- if (replicaManager != null) {
- replicaManager.shutdown();
- }
+ shutdownReplicaManager();
+ shutdownTabletManagers();
Review Comment:
These calls are in the same `try` block, so any exception from
`shutdownReplicaManager()` skips `shutdownTabletManagers()` and every cleanup
step after it.
One concrete path is `checkpointHighWatermarks()`: a checkpoint write
failure in one data directory is thrown after replica activity has already been
stopped, but it still prevents KV and log tablets in every directory from being
closed.
This deterministically turns the TabletServer shutdown into an unclean
shutdown. Local logs are not force-flushed, recovery checkpoints and
clean-shutdown markers are not written, and even healthy data directories must
go through unclean recovery on the next startup.
Please preserve the shutdown order but handle these steps separately: record
the ReplicaManager failure, continue shutting down the tablet managers and
remaining resources, and report the aggregated failure afterward. Please also
add a regression test where `shutdownReplicaManager()` fails and verify that
`shutdownTabletManagers()` still runs.
##########
fluss-server/src/main/java/org/apache/fluss/server/ServerBase.java:
##########
@@ -177,15 +181,19 @@ public void onFatalError(Throwable exception) {
exception);
if (ExceptionUtils.isJvmFatalError(exception)) {
System.exit(-1);
- } else {
- closeAsync(Result.FAILURE);
+ } else if (fatalErrorShutdownTriggered.compareAndSet(false, true)) {
FutureUtils.orTimeout(
getTerminationFuture(),
FATAL_ERROR_SHUTDOWN_TIMEOUT_MS,
TimeUnit.MILLISECONDS,
String.format(
"Waiting for %s shutting down timed out after %s
ms.",
getServerName(), FATAL_ERROR_SHUTDOWN_TIMEOUT_MS));
+
+ Thread shutdownThread =
+ new ExecutorThreadFactory(getServerName() +
"-fatal-error-shutdown")
+ .newThread(() -> closeAsync(Result.FAILURE));
+ shutdownThread.start();
Review Comment:
`isJvmFatalError()` does not include `OutOfMemoryError`, so an OOM—including
“unable to create new native thread”—takes this branch.
The one-shot shutdown flag is set before the new thread is created. If
`newThread()` or `start()` throws, shutdown never starts, and later fatal-error
callbacks are ignored because the flag is already set.
Please treat OOM as immediately fatal by using
`isJvmFatalOrOutOfMemoryError()`, or catch thread creation/start failures and
terminate the process as a fallback. Please also add a test covering this
failure path.
--
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]
