feat: introduce stopGracefullyOnNewSpec for Supervisors#19568
feat: introduce stopGracefullyOnNewSpec for Supervisors#19568Fly-Style wants to merge 2 commits into
Supervisors#19568Conversation
|
This is related to: #19541 |
SupervisorsSupervisors
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 5 of 5 changed files.
This is an automated review by Codex GPT-5.5
| metadataSupervisorManager.insert(id, new NoopSupervisorSpec(null, pair.rhs.getDataSources())); | ||
| } | ||
| pair.lhs.stop(true); | ||
| pair.lhs.stop(pair.lhs.stopGracefullyOnNewSpec()); |
There was a problem hiding this comment.
[P2] Do not use the new-spec stop policy for terminate
This helper is also called by stopAndRemoveSupervisor(id) with writeTombstone=true, so explicit supervisor terminate now passes stopGracefullyOnNewSpec() into stop(). The new default returns false, and an implementation may return false specifically to avoid killing tasks during spec replacement; on the public terminate path that means stop(false), which the Supervisor contract says leaves running tasks as-is even though the terminate API promises to stop associated tasks and publish segments. Please keep tombstone/terminate removal graceful, or split the replacement policy from the removal policy.
There was a problem hiding this comment.
Addressed, thanks!
2f26286 to
959998e
Compare
Generally, any supervisor should not terminate their tasks during spec update, but historically it happened that stream supervisors became bound to that logic. This PR introduces the correct behaviour - it supervisor stops gracefully, but stream supervisors do not via
stopGracefullyOnNewSpec()mechanism.It was done to enhance UX for existing and future supervisor kinds, and there are plans to enhance stream supervisors to stop gracefully too in a follow-up PR.
This PR has: