FrankChen021 commented on code in PR #19541:
URL: https://github.com/apache/druid/pull/19541#discussion_r3499365338
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -186,47 +188,81 @@ public boolean
createOrUpdateAndStartSupervisor(SupervisorSpec spec)
synchronized (lock) {
Preconditions.checkState(started, "SupervisorManager not started");
- final boolean shouldUpdateSpec = shouldUpdateSupervisor(spec);
- SupervisorSpec existingSpec =
possiblyStopAndRemoveSupervisorInternal(spec.getId(), false);
- spec.merge(existingSpec);
- createAndStartSupervisorInternal(spec, shouldUpdateSpec);
- return shouldUpdateSpec;
+
+ if (!skipRestartIfUnmodified) {
+ // Always stop/recreate, persisting whenever the spec actually changed
(or is new).
+ final boolean specChanged = isSpecChangedAndValidate(spec);
+ final SupervisorSpec existingSpec =
possiblyStopAndRemoveSupervisorInternal(spec.getId(), false);
+ spec.merge(existingSpec);
+ createAndStartSupervisorInternal(spec, specChanged);
+ return SupervisorSpecUpdateResult.of(specChanged, true);
+ }
+
+ final Pair<Supervisor, SupervisorSpec> current =
supervisors.get(spec.getId());
+ final boolean isNew = current == null || current.rhs == null;
+
+ if (isNew) {
+ spec.merge(null);
+ createAndStartSupervisorInternal(spec, true);
+ return SupervisorSpecUpdateResult.of(true, true);
+ }
+
+ if (!isSpecChanged(spec, current.rhs)) {
+ return SupervisorSpecUpdateResult.of(false, false);
+ }
+
+ // merge() may carry forward omitted fields (e.g. taskCount); compare
the effective spec.
+ spec.merge(current.rhs);
+ if (!isSpecChanged(spec, current.rhs)) {
+ return SupervisorSpecUpdateResult.of(false, false);
+ }
+
+ // The effective (merged) spec is what will be persisted, so validate
that transition exactly once.
+ current.rhs.validateSpecUpdateTo(spec);
+
+ if (!current.rhs.requireRestart(spec)) {
Review Comment:
[P2] No-restart task-count updates leave the live supervisor stale
When an autoscaling supervisor is resubmitted with only an effective
taskCount change, for example after autoscaling has moved the current
ioConfig.taskCount to 5 and the new POST is merged back to taskCountStart 2,
SeekableStreamSupervisorSpec.requireRestart masks taskCount differences
whenever either side has autoscaling enabled. This branch then persists the new
spec and swaps only the manager's stored spec, but it does not restart
current.lhs or update the running SeekableStreamSupervisor's ioConfig. The API
can return modified=true,restarted=false while ingestion continues using the
old taskCount until a later autoscaler action or Overlord restart. Treat this
reset as restart-required, or explicitly propagate the new taskCount into the
running supervisor before taking the no-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]
