Jackie-Jiang commented on code in PR #19265:
URL: https://github.com/apache/pinot/pull/19265#discussion_r3790492879
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -979,19 +990,26 @@ protected long getCurrentCountStarResult(String
tableName) {
}
protected void waitForMinionTaskCompletion(String taskId, long timeout) {
- TestUtils.waitForCondition(aVoid ->
-
_controllerStarter.getHelixTaskResourceManager().getTaskState(taskId) ==
TaskState.COMPLETED,
- timeout, "Failed to complete the task " + taskId);
-
- // Validate that there were > 0 subtasks so that we know the task was
actually run
-
Assert.assertFalse(_controllerStarter.getHelixTaskResourceManager().getSubtaskStates(taskId).isEmpty());
+ // The task state (workflow context) and the subtask states (job context)
live in different Helix znodes and are
+ // not updated atomically, so a subtask can still read as RUNNING (or
null, not yet started) right after the task
+ // turns COMPLETED. Wait until the task is COMPLETED and every subtask
reached a terminal state before validating
+ // them. A non-empty subtask map also proves the task was actually run.
+ PinotHelixTaskResourceManager taskResourceManager =
_controllerStarter.getHelixTaskResourceManager();
+ TestUtils.waitForCondition(aVoid -> {
+ if (taskResourceManager.getTaskState(taskId) != TaskState.COMPLETED) {
+ return false;
+ }
+ Map<String, TaskPartitionState> subtaskStates =
taskResourceManager.getSubtaskStates(taskId);
+ return !subtaskStates.isEmpty() && subtaskStates.values()
+ .stream()
+ .noneMatch(state -> state == null || state ==
TaskPartitionState.INIT || state == TaskPartitionState.RUNNING);
Review Comment:
Fixed in b45ae88: `STOPPED` is now treated as non-terminal in the wait
condition. You are right on the lifecycle — a `STOPPED` partition is resumable,
so under a `COMPLETED` task it can only be a stale read of a stop-then-resume
history, and exiting the wait on it re-opens the same race. Note this cannot
cause spurious timeouts in legitimate scenarios: a genuinely stopped task has
job state `STOPPED`, never `COMPLETED`, so the wait would already be blocked on
the task-state check.
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -329,22 +333,20 @@ protected Schema createSchema(File schemaFile)
protected TableConfig createTableConfig(String tableConfigFileName)
throws IOException {
URL configPathUrl =
getClass().getClassLoader().getResource(tableConfigFileName);
- Assert.assertNotNull(configPathUrl);
+ assertNotNull(configPathUrl);
return createTableConfig(new File(configPathUrl.getFile()));
}
protected TableConfig createTableConfig(File tableConfigFile)
throws IOException {
InputStream inputStream = new FileInputStream(tableConfigFile);
- Assert.assertNotNull(inputStream);
+ assertNotNull(inputStream);
return JsonUtils.inputStreamToObject(inputStream, TableConfig.class);
}
/// Creates a new OFFLINE table config.
protected TableConfig createOfflineTableConfig() {
- // @formatter:off
- return new TableConfigBuilder(TableType.OFFLINE)
- .setTableName(getTableName())
+ return new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName())
Review Comment:
The formatting cleanup is intentional: this is a test-infrastructure file,
and the reformat drops stale `@formatter:off` guards and normalizes builder
chains to the current repo style. Doing it in a commit that already touches the
file avoids a dedicated churn PR; the behavior change remains confined to
`waitForMinionTaskCompletion` and is easy to review in isolation.
--
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]
