DanielLeens commented on PR #12710: URL: https://github.com/apache/seatunnel/pull/12710#issuecomment-6391367704
Thanks @SEZ9 for the detailed follow-up. A couple of notes so the remaining asks are easy to close (I re-checked the current description and `SeaTunnelServer.shutdown(boolean terminate)` on `dev`, source-level only, nothing run locally): - **Policy / what "clean stop" means:** the description's Scope paragraph already states the result is a fast exit of the member via the Hazelcast shutdown hook, not a graceful drain of running jobs, and the user-facing section documents both the default `TERMINATE` behaviour and the `GRACEFUL` option together with the `hazelcast.graceful.shutdown.max.wait` < `terminationGracePeriodSeconds` constraint. `SeaTunnelServer.shutdown` indeed ignores the `terminate` argument, so switching the chart default to `GRACEFUL` would not make job stop cleaner, it would only add partition migration time to every pod stop. I would therefore keep `TERMINATE` as the chart default (current behaviour, no change for existing releases) and just make that intent explicit with one sentence in the values.yaml comment next to `terminationGracePeriodSeconds`. @ricky2129, could you add that? - **Follow-up ownership:** the graceful-job-stop work is still only referenced as "left for a follow-up". Please open a small issue for it (`SeaTunnelServer.shutdown` does not act on the terminate flag, so SIGTERM gives a fast member exit, not a graceful job stop) and link it from the description. Editing the description does not change the head, so it costs no CI. - **Grace-period default:** 30 equals the Kubernetes default, so existing releases are unchanged; a one-line note of the shutdown time you observed in the `docker stop` A/B would settle whether it is enough on a typical node. - **Docs (F3):** I checked the en/zh Kubernetes pages on this head: they lead with the exec-form `command` plus `terminationGracePeriodSeconds`, and `preStop` + `stop-seatunnel-cluster.sh` is presented as optional, only for setups that must keep `/bin/sh -c`. If you still read it as competing guidance, a pointer to the exact sentence would help. Separately, the Build check is still red on `96069fda` from the unrelated `BackpressureSlowSinkIT` flake in the fork run (`engine-v2-it (11)`), which is also why the merge queue dropped the PR. @ricky2129, please re-run only the failed job in the fork's Actions run; no code change is needed for that. -- 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]
