Copilot commented on code in PR #3698:
URL: https://github.com/apache/celeborn/pull/3698#discussion_r3297653317
##########
worker/src/test/scala/org/apache/celeborn/service/deploy/worker/WorkerStatusManagerSuite.scala:
##########
@@ -77,4 +77,30 @@ class WorkerStatusManagerSuite extends AnyFunSuite {
statusManager.doTransition(WorkerEventType.Recommission)
Assert.assertEquals(statusManager.getWorkerState(),
PbWorkerStatus.State.Normal)
}
+
+ test("Test exitEventType initialization based on config") {
+ // Default: neither graceful nor decommission → Immediately
+ val conf1 = new CelebornConf()
+ val mgr1 = new WorkerStatusManager(conf1)
+ Assert.assertEquals(WorkerEventType.Immediately, mgr1.exitEventType)
+
+ // Graceful shutdown only → Graceful
+ val conf2 = new CelebornConf()
+ conf2.set("celeborn.worker.graceful.shutdown.enabled", "true")
+ val mgr2 = new WorkerStatusManager(conf2)
+ Assert.assertEquals(WorkerEventType.Graceful, mgr2.exitEventType)
+
Review Comment:
In this suite, the earlier assertions use `Assert.assertEquals(actual,
expected)` (reversed vs JUnit’s `assertEquals(expected, actual)`), while this
new test uses the JUnit order. Mixing orders makes failure messages confusing;
please standardize the argument order within the suite (preferably `expected,
actual`).
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -1072,6 +1072,12 @@ private[celeborn] class Worker(
workerStatusManager.transitionState(State.Exit)
}
+ private val shutdownHookTimeout = if (conf.workerDecommissionShutdown) {
+ conf.workerDecommissionForceExitTimeout
Review Comment:
`shutdownHookTimeout` for decommission is set to
`workerDecommissionForceExitTimeout`, but `decommissionWorker()` can sleep past
that timeout by up to one `workerDecommissionCheckInterval` due to the `while
(waitTime < timeout) { Thread.sleep(interval); ... }` loop. This can cause
`ShutdownHookManager` to time out/cancel the hook before decommission finishes
(and before `stop(...)` runs). Consider capping the final sleep to the
remaining time or extending the shutdown hook timeout (e.g., `forceExitTimeout
+ checkInterval`, with bounds).
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -1094,7 +1100,9 @@ private[celeborn] class Worker(
}
},
"worker-shutdown-hook-thread"),
- WORKER_SHUTDOWN_PRIORITY)
+ WORKER_SHUTDOWN_PRIORITY,
+ shutdownHookTimeout,
+ TimeUnit.MILLISECONDS)
Review Comment:
With decommission-on-SIGTERM enabled, the shutdown hook can now execute
`decommissionWorker()`, but the subsequent `stop(...)` call still treats all
non-graceful paths as `CelebornExitKind.EXIT_IMMEDIATELY`. There is a dedicated
`CelebornExitKind.WORKER_DECOMMISSION`; using it for
`WorkerEventType.Decommission` would allow downstream components to apply
decommission-specific shutdown semantics instead of the immediate-exit behavior.
--
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]
