j1wonpark commented on PR #4197: URL: https://github.com/apache/amoro/pull/4197#issuecomment-4880839593
Thanks for the review @czy006! Both points are addressed: **1. `shutdown-timeout-ms` not wired from AMS** — Fixed in 153f8a2. `buildOptimizerStartupArgsString` now appends `-st` when the optimizer group sets the `shutdown-timeout-ms` property (same pattern as `-hb`), and `KubernetesOptimizerContainer` derives `terminationGracePeriodSeconds` from the same typed property via `PropertyUtil` instead of re-parsing the rendered CLI string (the string parser is removed). The JVM drain deadline and the pod grace period now come from a single source. Documented in `managing-optimizers.md`. **2. Flink drain hook** — Added in 7c762b6. `FlinkExecutor.close()` now stops the executor, waits up to the configured shutdown timeout for the in-flight task to finish (so its result is reported through the completeTask drain path), and only then force-interrupts. The drain budget is capped safely below Flink's cancellation watchdog (`task.cancellation.timeout`, default 180s) to avoid failing the whole TaskManager — documented together with the `-st` option. One known limitation on the Flink path: Flink cancels sources before downstream operators, so the toucher stops heartbeating when the drain begins and the effective drain window is additionally bounded by AMS `optimizer.heart-beat-timeout` (default 1 min). Fixing that properly needs AMS-side acceptance of a late `completeTask` for a task that was legitimately assigned (related to the stale-response handling in #4261) — I'd like to file a follow-up issue for that rather than grow this PR. While addressing the review I also found and fixed several defects in the new shutdown paths, in separate commits for easier review: - cc41ecb — interrupt-race hardening: unsafe publication of the executor thread array, the force-interrupt pass skipping threads when the stopping thread was itself interrupted, and a busy-spin regression from preserving the interrupt flag while still running. - f9a6061 — `completeTask` now retries through a bounded drain window after `stop()` instead of a single best-effort call, closing a race where a result produced during shutdown was silently dropped. - 5292316 — the toucher no longer re-registers while draining; scale-down unregisters the optimizer first, and re-registering created a ghost optimizer and rotated the executors' token so the final `completeTask` was rejected. - 4551aff — the graceful-shutdown test now waits deterministically for the task to start. Remaining gaps I'd propose as follow-ups (pre-existing container infra, not introduced here): the Spark container's release path (`spark-submit --kill`) doesn't extend the driver pod's termination grace period (K8s default 30s applies), and `LocalOptimizerContainer` releases with `kill -9`, which skips shutdown hooks entirely. -- 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]
