snleee commented on code in PR #10132:
URL: https://github.com/apache/pinot/pull/10132#discussion_r1073114137
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -451,6 +455,48 @@ public String getSubtaskProgress(@Context HttpHeaders
httpHeaders,
}
}
+ @GET
+ @Path("/tasks/subtask/state/{subTaskState}/progress")
Review Comment:
Using `subTaskState` as the path param doesn't look natural to me. I took a
look at other APIs and the pattern is to have `taskType or taskName` path
parameter.
Please give 5-10 min to see if there's a better way to represent this. If
feasible, I personally would still prefer to have a single API with all
different filters as query params.. (e.g. `GET /tasks/subtask/progress` <- this
can reuse the existing minion side API)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -451,6 +455,48 @@ public String getSubtaskProgress(@Context HttpHeaders
httpHeaders,
}
}
+ @GET
+ @Path("/tasks/subtask/state/{subTaskState}/progress")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation("Get progress of all subtasks with specified state tracked by
minion worker in memory")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500,
message = "Internal server error")
+ })
+ public String getSubtaskWithGivenStateProgress(@Context HttpHeaders
httpHeaders,
+ @ApiParam(value = "Subtask state
(UNKNOWN,IN_PROGRESS,SUCCEEDED,CANCELLED,ERROR)", required = true)
+ @PathParam("subTaskState") String subTaskState,
+ @ApiParam(value = "Minion work IDs separated by comma")
@QueryParam("minionWorkerIds") @Nullable
Review Comment:
`Minion work IDs` -> `Minion worker IDs`
##########
pinot-minion/src/main/java/org/apache/pinot/minion/api/resources/PinotTaskProgressResource.java:
##########
@@ -84,4 +86,34 @@ public String getSubtaskProgress(
.build());
}
}
+
+ @GET
+ @Path("/tasks/subtask/state/{subTaskState}/progress")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation("Get finer grained task progress tracked in memory for
subtasks with the given state")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500,
message = "Internal server error")
+ })
+ public String getSubtaskWithGivenStateProgress(
Review Comment:
I think that we can merge this function with `/tasks/subtask/progress`?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -451,6 +455,48 @@ public String getSubtaskProgress(@Context HttpHeaders
httpHeaders,
}
}
+ @GET
+ @Path("/tasks/subtask/state/{subTaskState}/progress")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation("Get progress of all subtasks with specified state tracked by
minion worker in memory")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500,
message = "Internal server error")
+ })
+ public String getSubtaskWithGivenStateProgress(@Context HttpHeaders
httpHeaders,
+ @ApiParam(value = "Subtask state
(UNKNOWN,IN_PROGRESS,SUCCEEDED,CANCELLED,ERROR)", required = true)
+ @PathParam("subTaskState") String subTaskState,
+ @ApiParam(value = "Minion work IDs separated by comma")
@QueryParam("minionWorkerIds") @Nullable
+ String minionWorkerIds) {
+ Set<String> selectedMinionWorkers = new HashSet<>();
+ if (StringUtils.isNotEmpty(minionWorkerIds)) {
+ Collections.addAll(selectedMinionWorkers,
StringUtils.split(minionWorkerIds, ','));
Review Comment:
I guess that `StringUtils.split()` would handle the trimming?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##########
@@ -419,7 +423,7 @@ public Map<String, PinotTaskConfig> getSubtaskConfigs(
@GET
@Path("/tasks/subtask/{taskName}/progress")
@Produces(MediaType.APPLICATION_JSON)
- @ApiOperation("Get progress of specified sub tasks for the given task
tracked by worker in memory")
+ @ApiOperation("Get progress of specified sub tasks for the given task
tracked by minion worker in memory")
Review Comment:
What if we return the task names and corresponding subtasks from
`/tasks/subtask/state/{subTaskState}/progress` and asks ppl to use
`/tasks/subtask/{taskName}/progress` to fetch for each progress of the subtask?
The existing API allows us to provide the list of subtasks so we can fetch the
information with a single call if needed.
--
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]
