SEZ9 commented on PR #12710: URL: https://github.com/apache/seatunnel/pull/12710#issuecomment-6392870933
Re-review of head `96069fda` done; since `f7ee365c` the only delta is the `dev` merge, so the PR-owned diff is unchanged and my earlier findings still apply as follows. **F1 / F2 — SIGTERM path and shutdown policy (master/worker deployments)** The description and the user-facing docs now state clearly that the result is a fast member exit via the Hazelcast shutdown hook (`TERMINATE`), not a graceful drain of running jobs, and they document the `GRACEFUL` option together with the `hazelcast.graceful.shutdown.max.wait` < `terminationGracePeriodSeconds` constraint. Given that `SeaTunnelServer.shutdown(boolean terminate)` does not act on the terminate flag, switching the chart default to `GRACEFUL` would only add partition-migration time to every pod stop without making job stop cleaner, so keeping `TERMINATE` as the chart default is the right call and preserves current behaviour for existing releases. Remaining asks: - Add one sentence to the `values.yaml` comment next to `terminationGracePeriodSeconds` (master and worker) making that intent explicit: default policy is `TERMINATE`, i.e. fast member exit, not graceful job stop. - Open a small follow-up issue for the graceful-job-stop work (`SeaTunnelServer.shutdown` ignoring the terminate flag) and link it from the description instead of the current "left for a follow-up" wording. This is description-only, so no new push is needed. - Please add a one-line note of the shutdown time you observed in the `docker stop` A/B so we can confirm the 30s default (same as the Kubernetes default) is sufficient on a typical node. **F3 — Kubernetes graceful-shutdown docs** Re-checked the en/zh pages on this head: they lead with the exec-form `command` plus `terminationGracePeriodSeconds`, and `preStop` + `stop-seatunnel-cluster.sh` is presented as optional for setups that must keep `/bin/sh -c`. That resolves my concern; I consider F3 addressed, no further change needed there. Once the `values.yaml` comment and the linked follow-up issue are in, I'm happy to approve. <!-- streview-comment:2017 --> -- 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]
