Copilot commented on code in PR #3698:
URL: https://github.com/apache/celeborn/pull/3698#discussion_r3295757601
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/WorkerStatusManager.scala:
##########
@@ -39,8 +39,13 @@ private[celeborn] class WorkerStatusManager(conf:
CelebornConf) extends Logging
private var worker: Worker = _
private var shutdown: AtomicBoolean = _
private var storageManager: StorageManager = _
+ private val decommissionShutdown = conf.workerDecommissionShutdown
private val gracefulShutdown = conf.workerGracefulShutdown
- if (gracefulShutdown) {
+ if (decommissionShutdown) {
+ exitEventType = WorkerEventType.Decommission
+ logInfo("Decommission shutdown enabled, worker will decommission on
SIGTERM" +
+ " (overrides graceful shutdown)")
+ } else if (gracefulShutdown) {
exitEventType = WorkerEventType.Graceful
}
Review Comment:
New behavior sets `exitEventType` to `Decommission` when
`celeborn.worker.decommission.shutdown.enabled` is true (and logs the
override). There are existing unit tests for `WorkerStatusManager` (e.g.,
`WorkerStatusManagerSuite`), but none assert this new initialization/override
behavior. Adding a small test for the config-driven `exitEventType` selection
(and graceful override) would prevent regressions.
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -1081,6 +1081,9 @@ private[celeborn] class Worker(
case WorkerEventType.Graceful =>
shutdownGracefully()
case WorkerEventType.Decommission =>
+ ShutdownHookManager.get().updateTimeout(
+ conf.workerDecommissionForceExitTimeout,
+ TimeUnit.MILLISECONDS)
decommissionWorker()
Review Comment:
`ShutdownHookManager.updateTimeout(...)` is called *inside* the shutdown
hook, but `ShutdownHookManager.executeShutdown()` reads the hook’s timeout
before running it (`future.get(entry.getTimeout(), ...)`). That means this
update will not extend the timeout for the currently executing worker hook, so
`decommissionWorker()` can still be interrupted after
`celeborn.worker.graceful.shutdown.timeout` (default 600s), which defeats the
purpose of aligning with `workerDecommissionForceExitTimeout` (default 6h).
Move the timeout update to a point *before* JVM shutdown starts (e.g.,
register the worker hook with an explicit timeout when decommission-on-SIGTERM
is enabled, or update the timeout when transitioning to
`WorkerEventType.Decommission` before calling `System.exit`).
--
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]
