Copilot commented on code in PR #17720:
URL: https://github.com/apache/pinot/pull/17720#discussion_r2820891368
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java:
##########
@@ -1260,8 +1290,10 @@ public void testTableTasksCleanupWithActiveTasks()
assertEquals(deleteResponse, "{\"status\":\"Tables: [" + tableName +
"_OFFLINE] deleted\"}");
// delete task
-
sendDeleteRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder().forDeleteMinionTask(taskName)
- + "?forceDelete=true");
+ sendPutRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder()
+
.forStopMinionTaskQueue(MinionConstants.SegmentGenerationAndPushTask.TASK_TYPE));
+ waitForTaskStateOrTaskMissing(taskName, TaskState.STOPPED);
+ deleteMinionTaskWithRetry(taskName);
Review Comment:
The test stops the minion task queue but never resumes it. This can leak
shared state into subsequent tests in this class/suite (note the earlier
cleanup test explicitly resumes the queue “to avoid affecting other tests”).
Please ensure the queue is resumed (ideally in a finally block or in
@AfterMethod cleanup) even if assertions fail.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java:
##########
@@ -1223,6 +1223,36 @@ private static void waitForTaskState(String taskName,
TaskState expectedState) {
}, 5000, "Task not scheduled");
}
+ private static void waitForTaskStateOrTaskMissing(String taskName, TaskState
expectedState) {
+ TestUtils.waitForCondition((aVoid) -> {
+ try {
+ return
sendGetRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder().forMinionTaskState(taskName))
+ .replace("\"", "")
+ .equals(expectedState.name());
+ } catch (IOException e) {
+ return isTaskStateNotFound(e);
+ }
+ }, 8000, "Task not stopped or removed");
+ }
+
+ private static void deleteMinionTaskWithRetry(String taskName) {
+ TestUtils.waitForCondition((aVoid) -> {
+ try {
+
sendDeleteRequest(DEFAULT_INSTANCE.getControllerRequestURLBuilder().forDeleteMinionTask(taskName)
+ + "?forceDelete=true");
+ return true;
+ } catch (IOException e) {
+ return isTaskStateNotFound(e);
+ }
+ }, 15000, "Failed to delete task: " + taskName);
+ }
+
+ private static boolean isTaskStateNotFound(IOException e) {
+ String message = e.getMessage();
+ return message != null && (message.contains("status code: 404") ||
message.contains("Not Found")
+ || message.contains("does not exist"));
+ }
Review Comment:
`isTaskStateNotFound` detects 404s by substring-matching
`IOException.getMessage()`, which is brittle and may misclassify errors if
message formats change. Since the request helpers wrap
`HttpErrorStatusException` as the IOException cause, prefer checking
`e.getCause()` for `HttpErrorStatusException` and using its status code (or a
dedicated helper that returns status codes) to reliably detect 404s.
--
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]
