Skip to content

feat: introduce stopGracefullyOnNewSpec for Supervisors#19568

Open
Fly-Style wants to merge 2 commits into
masterfrom
feat/stop-gracefully
Open

feat: introduce stopGracefullyOnNewSpec for Supervisors#19568
Fly-Style wants to merge 2 commits into
masterfrom
feat/stop-gracefully

Conversation

@Fly-Style

Copy link
Copy Markdown
Contributor

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:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

@jtuglu1

jtuglu1 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This is related to: #19541

@Fly-Style Fly-Style changed the title Introduce stopGracefullyOnNewSpec for Supervisors feat: introduce stopGracefullyOnNewSpec for Supervisors Jun 9, 2026

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks!

@Fly-Style Fly-Style force-pushed the feat/stop-gracefully branch from 2f26286 to 959998e Compare June 10, 2026 12:30
@Fly-Style Fly-Style requested review from gianm and kfaraz June 10, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants