feat: rework `requireRestart` to `getActionOnUpdateTo` to control restart/task-disruption behavior by Fly-Style · Pull Request #19700 · apache/druid

Fly-Style · 2026-07-17T14:12:20Z

Today, updating a supervisor spec always restarts the supervisor and terminates its running tasks, even when the change is cosmetic. This makes routine spec updates unnecessarily disruptive to in-flight ingestion.

This PR replaces the boolean SupervisorSpec#requireRestart with a tri-state SupervisorSpec#getActionOnUpdateTo(SupervisorSpec proposedSpec), returning a SupervisorSpecUpdateAction:

NONE - persist the new spec, no restart
RESTART_SUPERVISOR - restart the supervisor, but leave running tasks alone
RESTART_SUPERVISOR_AND_TASKS - restart the supervisor and terminate its tasks (previous behavior; still the default)

Copilot

Pull request overview

This PR adds a new SupervisorSpec extension point to control whether supervisor restarts (stop+recreate) should gracefully stop managed tasks, enabling certain supervisor implementations to avoid disrupting running tasks during spec updates while preserving existing default behavior.

Changes:

Added SupervisorSpec#shouldRestartCauseTaskDisruption() with a default of true to preserve existing behavior.
Threaded the new flag through SupervisorManager so restarts can call Supervisor.stop(...) with either disruptive (true) or non-disruptive (false) behavior, while termination continues to always disrupt tasks.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java`	Introduces the new default method used to decide whether restarts should disrupt managed tasks.
`indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java`	Wires the new flag into supervisor restart/stop flows by passing it through to `Supervisor.stop(...)`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kfaraz · 2026-07-17T14:45:56Z

+  /**
+   * Returns true if the supervisor restart should cause task disruption, false otherwise
+   */
+  default boolean shouldRestartCauseTaskDisruption()


Whether to restart tasks or not will depend on the difference between the current spec and proposed spec.

In some cases, we might want to rollover tasks and in other cases, we might want to continue with the existing tasks.

Suggested change

default boolean shouldRestartCauseTaskDisruption()

default boolean shouldRolloverTasksOnRestart(SupervisorSpec proposedSpec);

kfaraz · 2026-07-17T14:47:10Z

+  private SupervisorSpec possiblyStopAndRemoveSupervisorInternal(
+      String id,
+      boolean writeTombstone,
+      boolean shouldRestartCauseTaskDisruption


Suggested change

boolean shouldRestartCauseTaskDisruption

boolean terminateTasks

kfaraz · 2026-07-17T14:55:31Z

@Fly-Style , since the methods requireRestart and the new one (shouldRolloverTasksOnRestart) are closely related, I wonder if we shouldn't just have a single method that returns a tri-state enum.

The requireRestart method could then be changed to:

SupervisorAction getActionOnUpdateTo(SupervisorSpec proposedSpec);

enum SupervisorAction
{
  NONE, RESTART_SUPERVISOR, RESTART_SUPERVISOR_AND_TASKS
}

The advantage is that implementers would have to implement just one method and not need to worry about how the two methods play together. The end result would be much more intuitive.

cc: @jtuglu1 , do you have any thoughts?

FrankChen021

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

Reviewed 2 of 2 changed files.

Found one lifecycle race where an active autoscaler can still disrupt tasks during an opted-out restart.

This is an automated review by Codex GPT-5.6-Sol

kfaraz · 2026-07-20T09:17:20Z

+  default boolean shouldRolloverTasksOnRestart(SupervisorSpec proposedSpec)
+  {


@Fly-Style , thinking some more on this, let's change the existing method requireRestart instead of adding a new method.

ref: #19700 (comment)

Let's try to merge it off soon so that we can ensure it is include it in Druid 38 to avoid multiple changes to this interface since it an extension API.

kfaraz

Minor nitpicks, rest looks good!

kfaraz · 2026-07-20T11:11:42Z

        // Always stop/recreate, persisting whenever the spec actually changed (or is new).
        final boolean specChanged = isSpecChangedAndValidated(spec);
-        final SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false);
+        final Pair<Supervisor, SupervisorSpec> current = supervisors.get(spec.getId());


The skipRestartIfUnmodified flag is false by default.
So, in that case, I wonder if we shouldn't just retain current behaviour of always restarting the tasks.

Meanwhile, we still may have it configured with only supervisor restart.
I am voting to have the update logic, not retain what we have now.

Yeah, I was torn between the two options myself.

But then I realized the restart here really refers to the task restart itself, since that is where the churn happens. Restarting the supervisor itself is a cheap operation.

So when skipRestart is false, user doesn't want to skip restart at all, so we should restart both tasks and supervisors.

As for the default behaviour, we can update the default value of this flag in SupervisorResource in the next release (Druid 39).

Fly-Style · 2026-07-20T11:41:53Z

@kfaraz all comments were addressed

kfaraz

LGTM 🚀

kfaraz · 2026-07-20T11:42:49Z

 {
  private final boolean modified;
-  private final boolean restarted;
+  private final SupervisorSpecUpdateAction action;


Side thought: I wonder if this field will also get serialized out even though it doesn't have a getter.

kfaraz · 2026-07-20T11:49:31Z

Including this in Druid 38 to avoid unnecessary changes to the Supervisor interface between releases.

Introduce shouldRestartCauseTaskDisruption in SupervisorSpec

9865349

Fly-Style requested review from Copilot, jtuglu1 and kfaraz July 17, 2026 14:12

github-actions Bot added the Area - Ingestion label Jul 17, 2026

Copilot started reviewing on behalf of Fly-Style July 17, 2026 14:12 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

AI review suggestion

fccff34

kfaraz reviewed Jul 17, 2026

View reviewed changes

apply suggestion from inline comments

b6226f0

github-advanced-security AI found potential problems Jul 17, 2026

View reviewed changes

Comment thread server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java Fixed

FrankChen021 reviewed Jul 18, 2026

View reviewed changes

Comment thread ...g-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java

Fly-Style changed the title ~~feat: introduce shouldRestartCauseTaskDisruption in SupervisorSpec~~ feat: introduce shouldRolloverTasksOnRestart() in SupervisorSpec Jul 20, 2026

Fly-Style requested a review from kfaraz July 20, 2026 07:59

kfaraz requested changes Jul 20, 2026

View reviewed changes

Fly-Style added 2 commits July 20, 2026 12:26

Change the implementation to enum-based version

8af6798

Finish unfinished javadoc

442f874

Fly-Style changed the title ~~feat: introduce shouldRolloverTasksOnRestart() in SupervisorSpec~~ feat: rework requireRestart to getActionOnUpdateTo to control restart/task-disruption behavior Jul 20, 2026

Fly-Style requested a review from kfaraz July 20, 2026 09:41

kfaraz reviewed Jul 20, 2026

View reviewed changes

Post-review fixes

4080a16

Fly-Style requested a review from kfaraz July 20, 2026 11:46

kfaraz approved these changes Jul 20, 2026

View reviewed changes

kfaraz added this to the 38.0.0 milestone Jul 20, 2026

Fly-Style merged commit f0d8c61 into apache:master Jul 20, 2026
38 checks passed

Fly-Style deleted the supervisor-introduce-restart-quiet branch July 20, 2026 13:13

github-actions Bot removed this from the 38.0.0 milestone Jul 20, 2026

github-actions Bot added this to the 39.0.0 milestone Jul 20, 2026

github-actions Bot mentioned this pull request Jul 20, 2026

[38.0.0] feat: rework requireRestart to getActionOnUpdateTo to control restart/task-disruption behavior #19705

Open

	default boolean shouldRestartCauseTaskDisruption()
	default boolean shouldRolloverTasksOnRestart(SupervisorSpec proposedSpec);

	boolean shouldRestartCauseTaskDisruption
	boolean terminateTasks

Uh oh!

Conversation

Fly-Style commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz commented Jul 17, 2026

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfaraz Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fly-Style commented Jul 20, 2026

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfaraz commented Jul 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fly-Style commented Jul 17, 2026 •

edited

Loading

kfaraz Jul 20, 2026 •

edited

Loading

kfaraz Jul 20, 2026 •

edited

Loading