Update the Supervisor endpoint to not restart the Supervisor if the spec was unmodified #17707

aho135 · 2025-02-08T01:50:49Z

Description

This PR adds an optional query parameter called restartIfUnmodified to the /druid/indexer/v1/supervisor endpoint. The caller can optionally set restartIfUnmodified=false so that the supervisor is not restarted if the spec is unchanged. Multiple members of the community mentioned that they maintain their own scripts to check whether the spec has changed before submitting to the endpoint: https://apachedruidworkspace.slack.com/archives/C0303FDCZEZ/p1738017586080509

For those that rely on this endpoint for restarting the supervisor, the behavior remains unchanged as restartIfUnmodified defaults to true.

This PR also helps avoid unnecessary updates to the metastore when updating retention rules by first checking if the rules were updated.

Release note

Adds an optional query parameter called restartIfUnmodified to the /druid/indexer/v1/supervisor endpoint. Callers can set restartIfUnmodified=false to not restart the supervisor if the spec is unchanged. Example:

curl -X POST --header "Content-Type: application/json" -d @supervisor.json localhost:8888/druid/indexer/v1/supervisor?restartIfUnmodified=false

Key changed/added classes in this PR

SupervisorResource
SupervisorManager
SupervisorResourceTest
SQLMetadataRuleManager
SQLMetadataRuleManagerTest

This PR has:

…pec was unmodified

server/src/main/java/org/apache/druid/metadata/MetadataSupervisorManager.java

kfaraz

@aho135 , thanks for the PR. It makes sense to not update/restart the supervisor if not required.

I have left some minor feedback on the approach.

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

...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java

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

server/src/main/java/org/apache/druid/metadata/MetadataSupervisorManager.java

AmatyaAvadhanula · 2025-02-09T14:26:12Z

Thank you for these changes, @aho135!

I think we would benefit from a change where we check if the spec has changed. If it hasn't we still restart the supervisor, but do not go to the metadata store and add an unnecessary entry in the spec history. Otherwise, the flow remains unchanged. I think @kfaraz has suggested this as well.
I believe the supervisor restart itself would be helpful to rollover tasks easily or to get out of an idle supervisor state etc.

I also wanted to understand if the problem was with the metadata operations associated with it including an unneeded entry, or if the supervisor operation is also problematic.

If it is just the first case, is a feature flag really needed?
I believe we should skip the metadata operation and history update as there is no benefit in both cases

If you still believe that the supervisor operation is wasteful, and want to introduce a flag, please add the relevant docs in docs/api-reference/supervisor-api.md.

aho135 · 2025-02-10T19:00:16Z

Thank you for these changes, @aho135!

I think we would benefit from a change where we check if the spec has changed. If it hasn't we still restart the supervisor, but do not go to the metadata store and add an unnecessary entry in the spec history. Otherwise, the flow remains unchanged. I think @kfaraz has suggested this as well. I believe the supervisor restart itself would be helpful to rollover tasks easily or to get out of an idle supervisor state etc.

I also wanted to understand if the problem was with the metadata operations associated with it including an unneeded entry, or if the supervisor operation is also problematic.

If it is just the first case, is a feature flag really needed? I believe we should skip the metadata operation and history update as there is no benefit in both cases

If you still believe that the supervisor operation is wasteful, and want to introduce a flag, please add the relevant docs in docs/api-reference/supervisor-api.md.

Thanks for the review @AmatyaAvadhanula! My original motivation for this change was to avoid unnecessary restarts of the Supervisor if possible. Our use case is that we maintain a repository of schemas and do periodic releases. It is often unclear which schemas were actually modified. We want to be able to submit them all, and just restart the Supervisors which had schema updates. This is so we can avoid the undesirable side effects of task restart, such as small segments.

With this use case in mind, I think that having the feature flag does make sense. I will add an update in the relevant doc

…rvisorManager, update Supervisor API ref

aho135 · 2025-02-11T18:18:09Z

Hi @kfaraz @AmatyaAvadhanula I've made the changes you suggested and added some unit tests. Would appreciate if you could take a look again. Thank you!

aho135 · 2025-02-24T20:37:16Z

@kfaraz I've added an additional check for retention rules to avoid metastore updates if the retention rules were unchanged. This brings the behavior in line with the compaction config endpoint: https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorConfigManager.java#L110-L111

…rules are unchanged

JRobTS · 2025-02-25T23:28:30Z

Every language defaults boolean values to false and many treat null as false.

Maybe a small thing but I feel like the feature flag should default to false when not present. Introducing the new flag as something like skipRestartIfUnmodified=true would be better.

aho135 · 2025-02-26T01:46:12Z

Every language defaults boolean values to false and many treat null as false.

Maybe a small thing but I feel like the feature flag should default to false when not present. Introducing the new flag as something like skipRestartIfUnmodified=true would be better.

Thanks for the review @JRobTS Agreed that defaulting the flag to false makes more sense. I updated this in the subsequent PR. Please let me know what you think!

JRobTS · 2025-02-26T05:06:02Z

docs/api-reference/supervisor-api.md

-#### Sample request with restartIfUnmodified
-The following example sets the restartIfUnmodified flag to false. With this flag set to false, the Supervisor will only restart if there has been a modification to the SupervisorSpec. 
+#### Sample request with skipRestartIfUnmodified
+The following example sets the skipRestartIfUnmodified flag to true. With this flag set to false, the Supervisor will only restart if there has been a modification to the SupervisorSpec. 


Typo in the 2nd sentence: "With this flag set to false ..."

Thanks, good catch @JRobTS this was updated in the subsequent commit

kfaraz

Thanks for the update on this, @aho135 .

kfaraz · 2025-03-01T04:02:11Z

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

+      log.error(e,
+                "Failed to upgrade pending segment[%s] to new pending segment[%s] on Supervisor[%s].",
+                upgradedPendingSegment.getUpgradedFromSegmentId(),
+                upgradedPendingSegment.getId().getVersion(),
+                supervisorId
+      );


Suggested change

log.error(e,

"Failed to upgrade pending segment[%s] to new pending segment[%s] on Supervisor[%s].",

upgradedPendingSegment.getUpgradedFromSegmentId(),

upgradedPendingSegment.getId().getVersion(),

supervisorId

);

log.error(

e,

"Failed to upgrade pending segment[%s] to new pending segment[%s] on Supervisor[%s].",

upgradedPendingSegment.getUpgradedFromSegmentId(),

upgradedPendingSegment.getId().getVersion(),

supervisorId

);

kfaraz · 2025-03-01T04:03:26Z

...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java

+      @Context final HttpServletRequest req,
+      @QueryParam("skipRestartIfUnmodified") boolean skipRestartIfUnmodified


Style: Make the request the last argument of this method.

kfaraz · 2025-03-01T04:04:34Z

...-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java

+      @Context final HttpServletRequest req,
+      @QueryParam("skipRestartIfUnmodified") boolean skipRestartIfUnmodified


Use a boxed Boolean instead and handle nulls to make the default behaviour more obvious.

kfaraz · 2025-03-01T04:06:07Z

...rvice/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java

@@ -60,6 +62,7 @@
 @RunWith(EasyMockRunner.class)
 public class SupervisorManagerTest extends EasyMockSupport
 {
+  private static final ObjectMapper MAPPER = new DefaultObjectMapper();


add an empty line after this

kfaraz · 2025-03-01T04:06:24Z

...rvice/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java

+    manager.start();
+    Assert.assertFalse(manager.shouldUpdateSupervisor(spec));
+    Assert.assertTrue(manager.shouldUpdateSupervisor(spec2));
+  }


add an empty line after this.

kfaraz · 2025-03-01T04:08:29Z

website/.spelling

@@ -2409,6 +2409,7 @@ ddSketch
 DDSketch
 druid-ddsketch
 numBins
+skipRestartIfUnmodified


You wouldn't need this spelling entry if you use skipRestartIfUnmodified (with backquotes) instead of skipRestartIfUnmodified in the docs.

kfaraz · 2025-03-01T04:08:58Z

docs/api-reference/supervisor-api.md

@@ -2353,6 +2353,63 @@ Content-Length: 1359
 </TabItem>
 </Tabs>

+#### Sample request with skipRestartIfUnmodified
+The following example sets the skipRestartIfUnmodified flag to true. With this flag set to true, the Supervisor will only restart if there has been a modification to the SupervisorSpec. 


Suggested change

The following example sets the skipRestartIfUnmodified flag to true. With this flag set to true, the Supervisor will only restart if there has been a modification to the SupervisorSpec.

The following example sets the `skipRestartIfUnmodified` flag to true. With this flag set to true, the Supervisor will only restart if there has been a modification to the SupervisorSpec.

kfaraz · 2025-03-01T04:09:56Z

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

+   * Checks whether the submitted SupervisorSpec differs from the current spec in SupervisorManager's supervisor list.
+   * This is used in SupervisorResource specPost to determine whether the Supervisor needs to be restarted
+   * @param spec The spec submitted
+   * @return boolean - false if the spec is unchanged, else true


Suggested change

* @return boolean - false if the spec is unchanged, else true

* @return true only if the spec has been modified, false otherwise

kfaraz · 2025-03-01T04:11:05Z

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

+        if (currentSupervisor != null &&
+            Arrays.equals(specAsBytes, jsonMapper.writeValueAsBytes(currentSupervisor.rhs))
+        ) {
+          return false;
+        }


you may simplify this as follows:

Suggested change

if (currentSupervisor != null &&

Arrays.equals(specAsBytes, jsonMapper.writeValueAsBytes(currentSupervisor.rhs))

) {

return false;

}

return currentSupervisor == null

|| !Arrays.equals(specAsBytes, jsonMapper.writeValueAsBytes(currentSupervisor.rhs);

kfaraz · 2025-03-01T08:38:56Z

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

+   * @param spec The spec submitted
+   * @return boolean - false if the spec is unchanged, else true
+   */
+  public boolean shouldUpdateSupervisor(SupervisorSpec spec)


Based on the suggestion from @AmatyaAvadhanula , you can also update the createOrUpdate method to create a new entry in DB only if needed.

/** * Creates or updates a supervisor and then starts it. * If no change has been made to the supervisor spec, it is only restarted. * * @return true if the supervisor was updated, false otherwise */ public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) { Preconditions.checkState(started, "SupervisorManager not started"); Preconditions.checkNotNull(spec, "spec"); Preconditions.checkNotNull(spec.getId(), "spec.getId()"); Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()"); synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); // Persist a new version of the spec only if it has been updated final boolean shouldUpdateSpec = shouldUpdateSupervisor(spec); possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); createAndStartSupervisorInternal(spec, shouldUpdateSpec); return shouldUpdateSpec; } }

Update the Supervisor endpoint to not restart the Supervisor if the s…

e2800b3

…pec was unmodified

github-actions bot added the Area - Ingestion label Feb 8, 2025

github-advanced-security bot found potential problems Feb 8, 2025

View reviewed changes

server/src/main/java/org/apache/druid/metadata/MetadataSupervisorManager.java Fixed Show fixed Hide fixed

kfaraz reviewed Feb 9, 2025

View reviewed changes

Move SupervisorSpec comparison from MetadataSupervisorManager to Supe…

2bb4603

…rvisorManager, update Supervisor API ref

github-actions bot added the Area - Documentation label Feb 10, 2025

Andrew Ho added 2 commits February 10, 2025 13:47

Suppress spellcheck for restartIfUnmodified

a8db3de

Add unit tests

29dbd28

Add javadoc for shouldUpdateSupervisor

22d6a94

aho135 requested a review from kfaraz February 18, 2025 21:54

Add check to see if retention rules were updated

3463634

Andrew Ho added 2 commits February 24, 2025 14:57

Add unit tests to verify audit history is not updated when retention …

fe3954e

…rules are unchanged

Fix code formatting

34c5d80

Andrew Ho added 2 commits February 25, 2025 17:42

Update skipRestartIfUnmodified flag to default to false

47a0144

Fix doc

360310b

JRobTS reviewed Feb 26, 2025

View reviewed changes

kfaraz reviewed Mar 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the Supervisor endpoint to not restart the Supervisor if the spec was unmodified #17707

Update the Supervisor endpoint to not restart the Supervisor if the spec was unmodified #17707

aho135 commented Feb 8, 2025 •

edited

Loading

kfaraz left a comment

AmatyaAvadhanula commented Feb 9, 2025 •

edited

Loading

aho135 commented Feb 10, 2025

aho135 commented Feb 11, 2025

aho135 commented Feb 24, 2025

JRobTS commented Feb 25, 2025

aho135 commented Feb 26, 2025

JRobTS Feb 26, 2025

aho135 Feb 26, 2025

kfaraz left a comment

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

kfaraz Mar 1, 2025

		@Context final HttpServletRequest req,
		@QueryParam("skipRestartIfUnmodified") boolean skipRestartIfUnmodified

	The following example sets the skipRestartIfUnmodified flag to true. With this flag set to true, the Supervisor will only restart if there has been a modification to the SupervisorSpec.
	The following example sets the `skipRestartIfUnmodified` flag to true. With this flag set to true, the Supervisor will only restart if there has been a modification to the SupervisorSpec.

	* @return boolean - false if the spec is unchanged, else true
	* @return true only if the spec has been modified, false otherwise

Update the Supervisor endpoint to not restart the Supervisor if the spec was unmodified #17707

Are you sure you want to change the base?

Update the Supervisor endpoint to not restart the Supervisor if the spec was unmodified #17707

Conversation

aho135 commented Feb 8, 2025 • edited Loading

Description

Release note

Key changed/added classes in this PR

kfaraz left a comment

Choose a reason for hiding this comment

AmatyaAvadhanula commented Feb 9, 2025 • edited Loading

aho135 commented Feb 10, 2025

aho135 commented Feb 11, 2025

aho135 commented Feb 24, 2025

JRobTS commented Feb 25, 2025

aho135 commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aho135 commented Feb 8, 2025 •

edited

Loading

AmatyaAvadhanula commented Feb 9, 2025 •

edited

Loading