kfaraz commented on code in PR #17707:
URL: https://github.com/apache/druid/pull/17707#discussion_r1976267015
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java:
##########
@@ -119,7 +119,11 @@ public SupervisorResource(
@POST
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
- public Response specPost(final SupervisorSpec spec, @Context final
HttpServletRequest req)
+ public Response specPost(
+ final SupervisorSpec spec,
+ @Context final HttpServletRequest req,
+ @QueryParam("skipRestartIfUnmodified") boolean skipRestartIfUnmodified
Review Comment:
Use a boxed `Boolean` instead and handle nulls to make the default behaviour
more obvious.
##########
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.
Review Comment:
```suggestion
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.
```
##########
indexing-service/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();
Review Comment:
add an empty line after this
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -363,8 +399,12 @@ public boolean registerUpgradedPendingSegmentOnSupervisor(
return true;
}
catch (Exception e) {
- 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
+ );
Review Comment:
```suggestion
log.error(
e,
"Failed to upgrade pending segment[%s] to new pending
segment[%s] on Supervisor[%s].",
upgradedPendingSegment.getUpgradedFromSegmentId(),
upgradedPendingSegment.getId().getVersion(),
supervisorId
);
```
##########
website/.spelling:
##########
@@ -2409,6 +2409,7 @@ ddSketch
DDSketch
druid-ddsketch
numBins
+skipRestartIfUnmodified
Review Comment:
You wouldn't need this spelling entry if you use `skipRestartIfUnmodified`
(with backquotes) instead of skipRestartIfUnmodified in the docs.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java:
##########
@@ -119,7 +119,11 @@ public SupervisorResource(
@POST
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
- public Response specPost(final SupervisorSpec spec, @Context final
HttpServletRequest req)
+ public Response specPost(
+ final SupervisorSpec spec,
+ @Context final HttpServletRequest req,
+ @QueryParam("skipRestartIfUnmodified") boolean skipRestartIfUnmodified
Review Comment:
Style: Make the request the last argument of this method.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -166,6 +172,36 @@ public boolean
createOrUpdateAndStartSupervisor(SupervisorSpec spec)
}
}
+ /**
+ * 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
Review Comment:
```suggestion
* @return true only if the spec has been modified, false otherwise
```
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java:
##########
@@ -175,6 +178,21 @@ public void
testCreateOrUpdateAndStartSupervisorNullSpecId()
verifyAll();
}
+ @Test
+ public void testShouldUpdateSupervisor()
+ {
+ SupervisorSpec spec = new TestSupervisorSpec("id1", supervisor1);
+ SupervisorSpec spec2 = new TestSupervisorSpec("id2", supervisor2);
+ Map<String, SupervisorSpec> existingSpecs = ImmutableMap.of(
+ "id1", spec
+ );
+
EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs);
+ supervisor1.start();
+ replayAll();
+ manager.start();
+ Assert.assertFalse(manager.shouldUpdateSupervisor(spec));
+ Assert.assertTrue(manager.shouldUpdateSupervisor(spec2));
+ }
Review Comment:
add an empty line after this.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -166,6 +172,36 @@ public boolean
createOrUpdateAndStartSupervisor(SupervisorSpec spec)
}
}
+ /**
+ * 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
+ */
+ public boolean shouldUpdateSupervisor(SupervisorSpec spec)
Review Comment:
Based on the suggestion from @AmatyaAvadhanula , you can also update the
`createOrUpdate` method to create a new entry in DB only if needed.
```java
/**
* 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;
}
}
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -166,6 +172,36 @@ public boolean
createOrUpdateAndStartSupervisor(SupervisorSpec spec)
}
}
+ /**
+ * 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
+ */
+ public boolean shouldUpdateSupervisor(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");
+ try {
+ byte[] specAsBytes = jsonMapper.writeValueAsBytes(spec);
+ Pair<Supervisor, SupervisorSpec> currentSupervisor =
supervisors.get(spec.getId());
+ if (currentSupervisor != null &&
+ Arrays.equals(specAsBytes,
jsonMapper.writeValueAsBytes(currentSupervisor.rhs))
+ ) {
+ return false;
+ }
Review Comment:
you may simplify this as follows:
```suggestion
return currentSupervisor == null
|| !Arrays.equals(specAsBytes,
jsonMapper.writeValueAsBytes(currentSupervisor.rhs);
```
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
