snleee commented on code in PR #10132:
URL: https://github.com/apache/pinot/pull/10132#discussion_r1083379612


##########
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,

Review Comment:
   Do you think that it's better to group the result by taskName? 
   ```
   workerid -> taskName -> subtasks
   ```
   
   Or, we may want to filter based on the taskName as well?



##########
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:
   I think that it's fine to keep the existing approach given that it's for 
debugging purpose.. I don't have a strong opinion here.



##########
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:
   This looks to be not updated? Can you push your most recent code change?



##########
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:
   `GET /tasks/subtask/progress` with `minion worker id / sub task state` 
filter looks fine to me. I cannot come up with the better naming 😄 
   
   Maybe `GET /tasks/subtasks/workers/progress`?



-- 
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]

Reply via email to