suneet-s commented on code in PR #16310:
URL: https://github.com/apache/druid/pull/16310#discussion_r1597186247
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java:
##########
@@ -395,6 +395,25 @@ public Response shutdown(@PathParam("id") final String id)
return terminate(id);
}
+ @POST
+ @Path("/{id}/taskGroups/restart")
Review Comment:
```suggestion
@Path("/{id}/taskGroups/handoff")
```
^ maybe this is a good name to match the function name.
Can you also please add javadocs for this function explaining that it is
best effort. I think it is worth calling out that if a user passes in a
taskGroup that does not exist, it will not error here.
And some user visible docs here
https://github.com/apache/druid/blob/master/docs/api-reference/supervisor-api.md
##########
server/src/main/java/org/apache/druid/indexing/overlord/supervisor/Supervisor.java:
##########
@@ -93,4 +93,9 @@ default Boolean isHealthy()
LagStats computeLagStats();
int getActiveTaskGroupsCount();
+
+ /** Handoff the task group with id=taskGroupId the next time the supervisor
runs regardless of task run time*/
+ default void handoffTaskGroupEarly(int taskGroupId)
+ {
+ }
Review Comment:
```suggestion
default void handoffTaskGroupEarly(int taskGroupId) throws DruidException
{
throw DruidException
.forPersona(DruidException.Persona.ADMIN)
.ofCategory(DruidException.Category.UNSUPPORTED)
.build(...);
}
```
I think the default implementation should indicate in some way that this is
not supported. I don't have a strong opinion on if it needs to be via an
exception or some other mechanism.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -657,6 +669,37 @@ public String getType()
}
}
+ private class HandoffTaskGroupNotice implements Notice
+ {
+ final Integer taskGroupId;
Review Comment:
```suggestion
final List<Integer> taskGroupId;
```
I think accepting a List of integers in the notice will make for nicer dev
ergonomics
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java:
##########
@@ -395,6 +395,25 @@ public Response shutdown(@PathParam("id") final String id)
return terminate(id);
}
+ @POST
+ @Path("/{id}/taskGroups/restart")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ResourceFilters(SupervisorResourceFilter.class)
+ public Response handoffTaskGroups(@PathParam("id") final String id, final
List<Integer> taskGroupIds)
+ {
+ return asLeaderWithSupervisorManager(
+ manager -> {
+ if (manager.handoffTaskGroupsEarly(id, taskGroupIds)) {
Review Comment:
Should handle error case when the supervisor does not support
handoffTaskGroupsEarly
We probably also need a null or empty check for taskGroupIds.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -3132,7 +3153,7 @@ private void checkTaskDuration() throws
ExecutionException, InterruptedException
} else {
DateTime earliestTaskStart = computeEarliestTaskStartTime(group);
- if
(earliestTaskStart.plus(ioConfig.getTaskDuration()).isBeforeNow()) {
+ if
(earliestTaskStart.plus(ioConfig.getTaskDuration()).isBeforeNow() ||
group.getShutdownEarly()) {
Review Comment:
Please add a comment in the code explaining why we chose to ignore
stopTaskCount when `shutdownEarly` is set to true. I think it makes sense to
not respect it in this case, since stopTaskCount was meant to reduce task
spikes during normal operation, and shutdownEarly is in response to an API call
which is not a "normal" operation.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -657,6 +669,37 @@ public String getType()
}
}
+ private class HandoffTaskGroupNotice implements Notice
+ {
+ final Integer taskGroupId;
+ private static final String TYPE = "handoff_task_group_notice";
+
+ HandoffTaskGroupNotice(
+ final Integer taskGroupId
Review Comment:
Add Nonnull since it should be validated elsewhere
```suggestion
@Nonnull final List<Integer> taskGroupIds
```
--
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]
