kfaraz commented on code in PR #17707:
URL: https://github.com/apache/druid/pull/17707#discussion_r1978565879
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java:
##########
@@ -175,6 +178,22 @@ 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));
Review Comment:
There should also be a case where the method returns for an existing but
modified spec.
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataRuleManagerTest.java:
##########
@@ -115,6 +115,52 @@ public void testRuleInsert()
Assert.assertEquals(rules.get(0),
allRules.get(TestDataSource.WIKI).get(0));
}
+ @Test
+ public void testAuditEntryNotCreatedWhenRetentionRulesUnchanged()
Review Comment:
thanks for adding this.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java:
##########
@@ -151,6 +155,9 @@ public Response specPost(final SupervisorSpec spec,
@Context final HttpServletRe
if (!authResult.allowAccessWithNoRestriction()) {
throw new ForbiddenException(authResult.getErrorMessage());
}
+ if (skipRestartIfUnmodified != null && skipRestartIfUnmodified &&
!manager.shouldUpdateSupervisor(spec)) {
Review Comment:
Nit:
```suggestion
if (Boolean.TRUE.equals(skipRestartIfUnmodified) &&
!manager.shouldUpdateSupervisor(spec)) {
```
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java:
##########
@@ -555,7 +574,7 @@ public void
testGetActiveSupervisorIdForDatasourceWithAppendLock()
EasyMock.replay(activeSpec);
metadataSupervisorManager.insert(EasyMock.anyString(),
EasyMock.anyObject());
- SeekableStreamSupervisorSpec activeSpecWithConcurrentLocks =
EasyMock.mock(SeekableStreamSupervisorSpec.class);
+ SeekableStreamSupervisorSpec activeSpecWithConcurrentLocks =
EasyMock.createNiceMock(SeekableStreamSupervisorSpec.class);
Review Comment:
Why did these have to be changed?
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java:
##########
@@ -171,12 +171,61 @@ public List<String> getDataSources()
EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent());
replayAll();
- response = supervisorResource.specPost(spec, request);
+ response = supervisorResource.specPost(spec, false, request);
verifyAll();
Assert.assertEquals(503, response.getStatus());
}
+ @Test
+ public void testSpecPostskipRestartIfUnmodifiedTrue()
+ {
+ SupervisorSpec spec = new TestSupervisorSpec("my-id", null, null)
+ {
+
+ @Override
+ public List<String> getDataSources()
+ {
+ return Collections.singletonList("datasource1");
Review Comment:
Minor suggestion:
```suggestion
return List.of("datasource1");
```
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java:
##########
@@ -241,7 +290,7 @@ public List<String> getDataSources()
EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true);
replayAll();
- Assert.assertThrows(ForbiddenException.class, () ->
supervisorResource.specPost(spec, request));
+ Assert.assertThrows(ForbiddenException.class, () ->
supervisorResource.specPost(spec, false, request));
Review Comment:
There should also be usages which pass `null` for the new parameter.
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java:
##########
@@ -171,12 +171,61 @@ public List<String> getDataSources()
EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent());
replayAll();
- response = supervisorResource.specPost(spec, request);
+ response = supervisorResource.specPost(spec, false, request);
verifyAll();
Assert.assertEquals(503, response.getStatus());
}
+ @Test
+ public void testSpecPostskipRestartIfUnmodifiedTrue()
Review Comment:
```suggestion
public void testSpecPost_whenSkipRestartIfUnmodifiedIsTrue()
```
##########
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:
We should also call out the default value of the parameter.
Also include the parameter in the URL above.
--
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]
