Copilot commented on code in PR #19700:
URL: https://github.com/apache/druid/pull/19700#discussion_r3603822274
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java:
##########
@@ -148,4 +148,12 @@ default boolean requireRestart(SupervisorSpec proposedSpec)
{
return true;
}
+
+ /**
+ * Returns true if the supervisor restart should cause task disruption,
false otherwise
+ */
Review Comment:
The Javadoc here is a bit ambiguous about *when* this flag is consulted and
how it maps to actual supervisor behavior. Since the value is passed to
`Supervisor#stop(boolean)` (as the `stopGracefully` argument), it would help to
document that explicitly (and clarify that termination still always disrupts
tasks).
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -692,7 +700,7 @@ private boolean
possiblySuspendOrResumeSupervisorInternal(String id, boolean sus
}
SupervisorSpec nextState = suspend ? pair.rhs.createSuspendedSpec() :
pair.rhs.createRunningSpec();
- possiblyStopAndRemoveSupervisorInternal(nextState.getId(), false);
+ possiblyStopAndRemoveSupervisorInternal(nextState.getId(), false,
nextState.shouldRestartCauseTaskDisruption());
return createAndStartSupervisorInternal(nextState, true);
Review Comment:
`suspendOrResumeSupervisor` is documented as suspending indexing tasks until
resume. If `shouldRestartCauseTaskDisruption()` is overridden to return
`false`, this suspend/resume path would call `Supervisor.stop(false)`, which
(per `Supervisor#stop`) leaves running tasks as-is and would not actually
suspend indexing tasks.
This flag seems appropriate for spec-update restarts, but suspension should
always disrupt/stop tasks to match the API contract.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -197,7 +197,11 @@ public SupervisorSpecUpdateResult
createOrUpdateAndStartSupervisor(
if (!skipRestartIfUnmodified) {
// 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 SupervisorSpec existingSpec =
possiblyStopAndRemoveSupervisorInternal(
+ spec.getId(),
+ false,
+ spec.shouldRestartCauseTaskDisruption()
+ );
Review Comment:
In the legacy restart path (`skipRestartIfUnmodified=false`), the disruption
flag is evaluated on the *unmerged* user-submitted `spec`, but
`merge(existingSpec)` is allowed to carry forward omitted fields (and could
also influence the effective disruption behavior). This can lead to stopping
the existing supervisor with the wrong `stopGracefully` value (and also stops
the running supervisor before `merge` succeeds).
Consider merging with the current spec first, then computing
`shouldRestartCauseTaskDisruption()` from the effective merged spec before
stopping the old supervisor.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -234,7 +238,7 @@ public SupervisorSpecUpdateResult
createOrUpdateAndStartSupervisor(
}
// Restart path: stop+recreate, persisting the changed spec.
- possiblyStopAndRemoveSupervisorInternal(spec.getId(), false);
+ possiblyStopAndRemoveSupervisorInternal(spec.getId(), false,
spec.shouldRestartCauseTaskDisruption());
createAndStartSupervisorInternal(spec, true);
Review Comment:
This change introduces a new behavior knob (potentially calling
`Supervisor.stop(false)` during restart). There should be a unit test covering
the non-disruptive path (a `SupervisorSpec` overriding
`shouldRestartCauseTaskDisruption()` to return `false`) to assert that
restart/update calls stop with `false` and preserves the existing tasks
behavior.
Suggested location: `SupervisorManagerTest` with a spec update that triggers
the restart path.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
