Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+233 −11
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e2800b3
Update the Supervisor endpoint to not restart the Supervisor if the s…
Feb 8, 2025
2bb4603
Move SupervisorSpec comparison from MetadataSupervisorManager to Supe…
Feb 10, 2025
a8db3de
Suppress spellcheck for restartIfUnmodified
Feb 10, 2025
29dbd28
Add unit tests
Feb 11, 2025
22d6a94
Add javadoc for shouldUpdateSupervisor
Feb 11, 2025
3463634
Add check to see if retention rules were updated
Feb 24, 2025
fe3954e
Add unit tests to verify audit history is not updated when retention …
Feb 24, 2025
34c5d80
Fix code formatting
Feb 24, 2025
47a0144
Update skipRestartIfUnmodified flag to default to false
Feb 26, 2025
360310b
Fix doc
Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/api-reference/supervisor-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2353,10 +2353,10 @@ Content-Length: 1359
</TabItem>
</Tabs>

#### 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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```shell
curl "http://ROUTER_IP:ROUTER_PORT/druid/indexer/v1/supervisor?restartIfUnmodified=false" \
curl "http://ROUTER_IP:ROUTER_PORT/druid/indexer/v1/supervisor?skipRestartIfUnmodified=true" \
--header 'Content-Type: application/json' \
--data '{
"type": "kafka",
Expand Down
5 changes: 2 additions & 3 deletions ...rvice/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.Consumes;
import javax.ws.rs.DefaultValue;
import javax.ws.rs.GET;
import javax.ws.rs.POST;
import javax.ws.rs.Path;
Expand Down Expand Up @@ -123,7 +122,7 @@ public SupervisorResource(
public Response specPost(
final SupervisorSpec spec,
@Context final HttpServletRequest req,
@QueryParam("restartIfUnmodified") @DefaultValue("true") boolean restartIfUnmodified
@QueryParam("skipRestartIfUnmodified") boolean skipRestartIfUnmodified
)
{
return asLeaderWithSupervisorManager(
Expand Down Expand Up @@ -156,7 +155,7 @@ public Response specPost(
if (!authResult.allowAccessWithNoRestriction()) {
throw new ForbiddenException(authResult.getErrorMessage());
}
if (!restartIfUnmodified && !manager.shouldUpdateSupervisor(spec)) {
if (skipRestartIfUnmodified && !manager.shouldUpdateSupervisor(spec)) {
return Response.ok(ImmutableMap.of("id", spec.getId())).build();
}

Expand Down
16 changes: 8 additions & 8 deletions ...e/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public List<String> getDataSources()

replayAll();

Response response = supervisorResource.specPost(spec, request, true);
Response response = supervisorResource.specPost(spec, request, false);
verifyAll();

Assert.assertEquals(200, response.getStatus());
Expand All @@ -171,14 +171,14 @@ public List<String> getDataSources()
EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent());
replayAll();

response = supervisorResource.specPost(spec, request, true);
response = supervisorResource.specPost(spec, request, false);
verifyAll();

Assert.assertEquals(503, response.getStatus());
}

@Test
public void testSpecPostrestartIfUnmodifiedFalse()
public void testSpecPostskipRestartIfUnmodifiedTrue()
{
SupervisorSpec spec = new TestSupervisorSpec("my-id", null, null)
{
Expand All @@ -198,7 +198,7 @@ public List<String> getDataSources()
EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true);
replayAll();

Response response = supervisorResource.specPost(spec, request, false);
Response response = supervisorResource.specPost(spec, request, true);
verifyAll();

Assert.assertEquals(200, response.getStatus());
Expand All @@ -219,7 +219,7 @@ public List<String> getDataSources()

replayAll();

response = supervisorResource.specPost(spec, request, false);
response = supervisorResource.specPost(spec, request, true);
verifyAll();

Assert.assertEquals(200, response.getStatus());
Expand Down Expand Up @@ -250,7 +250,7 @@ public List<String> getDataSources()
EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true);
replayAll();

Response response = supervisorResource.specPost(spec, request, true);
Response response = supervisorResource.specPost(spec, request, false);
verifyAll();

Assert.assertEquals(200, response.getStatus());
Expand All @@ -260,7 +260,7 @@ public List<String> getDataSources()
EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent());
replayAll();

response = supervisorResource.specPost(spec, request, true);
response = supervisorResource.specPost(spec, request, false);
verifyAll();

Assert.assertEquals(503, response.getStatus());
Expand Down Expand Up @@ -290,7 +290,7 @@ public List<String> getDataSources()
EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true);
replayAll();

Assert.assertThrows(ForbiddenException.class, () -> supervisorResource.specPost(spec, request, true));
Assert.assertThrows(ForbiddenException.class, () -> supervisorResource.specPost(spec, request, false));
verifyAll();
}

Expand Down
2 changes: 1 addition & 1 deletion website/.spelling
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2409,7 +2409,7 @@ ddSketch
DDSketch
druid-ddsketch
numBins
restartIfUnmodified
skipRestartIfUnmodified

- ../docs/development/extensions-contrib/spectator-histogram.md
SpectatorHistogram
Expand Down
Loading