dongjoon-hyun commented on PR #58131: URL: https://github.com/apache/spark/pull/58131#issuecomment-5347928235
One more item I meant to include in my review above. It is a follow-up thought rather than a blocker, and it does not change my approval. When `decommissionExecutorsIfIdle` rejects every candidate, `ExecutorAllocationManager.removeExecutors` falls into the `else` branch and logs at WARN: > Unable to reach the cluster manager to kill executor/s ... or no executor eligible to kill! The message is now emitted for a routine, expected outcome, and it is misleading in that case - the cluster manager was reached, and the executor was deliberately skipped. It can also repeat. `ExecutorMonitor.timedOutExecutors()` caches `timedOutExecs` and rescans only once `now >= nextTimeout`, and `updateNextTimeout` only ever lowers `nextTimeout`. The task-start event that made the executor busy sets `timeoutAt` to `Long.MaxValue`, so it does not lower `nextTimeout` and does not trigger a rescan by itself. The rejected executor therefore stays in the cached candidate list and `schedule()` retries it every `intervalMillis` (100 ms). In the worst case - no other executor has a pending finite deadline, so `nextTimeout` stays `Long.MaxValue` - that continues until `onTaskEnd` lowers it, i.e. for the whole duration of the racing task. To be clear, this PR does not introduce it. The `decommissionEnabled = false` path already behaves exactly this way, since `killExecutors(force = false)` rejects busy executors and reaches the same branch. This patch just makes the decommission path share the existing wart, which is the correct trade-off for the bug being fixed here. So nothing to change in this PR. It seems worth a separate JIRA though - either a distinct, lower-level message for the "no executor eligible" case, or letting the monitor drop rejected candidates so the retry backs off. I'm happy to file it if you'd rather keep this PR focused. -- 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]
