SteNicholas commented on code in PR #3727:
URL: https://github.com/apache/celeborn/pull/3727#discussion_r3396227291
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -986,7 +986,7 @@ private[celeborn] class Worker(
shutdown.set(true)
val interval = conf.workerGracefulShutdownCheckSlotsFinishedInterval
val timeout = conf.workerGracefulShutdownCheckSlotsFinishedTimeoutMs
- var waitTimes = 0
+ var waitTimes = 1
Review Comment:
**Prefer keeping `waitTimes = 0` and changing the loop guard instead.**
With `waitTimes = 1`, the loop performs `waitTimes - 1` sleeps but `waitTime
= waitTimes * interval`, so the post-loop log over-reports the real wait by
exactly one `interval`:
- if all PartitionLocations are already released, the loop runs 0 times and
line 1000 logs `cost 1000ms` for a 0ms wait;
- in general the cost at lines 1000/1002 is always inflated by one
`interval`.
This undercuts the PR's own goal of accurate shutdown diagnostics. The
behavior described in the PR ("new condition will be `timeSpent + interval <
timeout`") is cleaner and avoids it — keep `waitTimes = 0` and change the guard:
```scala
while (!partitionLocationInfo.isEmpty && waitTime + interval < timeout) {
```
That leaves one `interval` of headroom for the post-loop logging/`stop()`
while keeping the reported cost exact.
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -1027,7 +1027,7 @@ private[celeborn] class Worker(
shutdown.set(true)
val interval = conf.workerDecommissionCheckInterval
val timeout = conf.workerDecommissionForceExitTimeout
- var waitTimes = 0
+ var waitTimes = 1
Review Comment:
Same as in `shutdownGracefully` — prefer keeping `waitTimes = 0` and using
`while (!… && waitTime + interval < timeout)`. Three notes specific to
decommission:
1. **Over-reported cost:** with `waitTimes = 1`, if the shuffle set is
already empty the loop runs 0 times and line 1041 logs `cost 30000ms` (one
`interval`) for a 0ms wait.
2. **Zero wait when `interval >= timeout`:** `waitTime` now starts at
`interval`, so if `celeborn.worker.decommission.checkInterval >=
celeborn.worker.decommission.forceExitTimeout` the guard is false on entry and
the worker skips the drain wait entirely, exiting while shuffles are still
unreleased (risking data loss / FetchFailed). With `waitTimes = 0` at least one
check+sleep always ran. The `waitTime + interval < timeout` form keeps this
property, so clamp to ≥1 iteration if that misconfig is a concern.
3. **Headroom vs. hook budget:** for decommission the shutdown-hook timeout
equals this loop's `timeout`
(`updateTimeout(workerDecommissionForceExitTimeout)`, Worker.scala:952). The
one-`interval` buffer this PR adds isn't reserved for
`sendWorkerDecommissionToMaster()`'s `askSync` before the loop or `stop()`
after it — under a small interval those can still exceed the budget and get
cancelled, which is the symptom you're fixing. A more robust fix reserves an
explicit shutdown buffer (loop timeout = hook timeout − buffer).
--
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]
