Copilot commented on code in PR #20306:
URL: https://github.com/apache/druid/pull/20306#discussion_r3969568146
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/AbstractParallelIndexSupervisorTaskTest.java:
##########
@@ -251,12 +251,21 @@ protected ParallelIndexTuningConfig newTuningConfig(
.withPartitionsSpec(partitionsSpec)
.withForceGuaranteedRollup(forceGuaranteedRollup)
.withMaxNumConcurrentSubTasks(maxNumConcurrentSubTasks)
- // Serial tests need only a short poll interval;
concurrent tests retain the default.
-
.withTaskStatusCheckPeriodMs(maxNumConcurrentSubTasks == 1 ? 100L : null)
+
.withTaskStatusCheckPeriodMs(getTaskStatusCheckPeriodMs(maxNumConcurrentSubTasks))
.withMaxParseExceptions(5)
.build();
}
+ /**
+ * Task-status poll interval used by {@link #newTuningConfig}. Serial tests
need only a short poll interval;
+ * concurrent tests retain the production default (null). Subclasses whose
assertions do not depend on poll
+ * cadence can override this to cut wall-clock time without duplicating the
rest of the tuning config.
+ */
+ protected Long getTaskStatusCheckPeriodMs(int maxNumConcurrentSubTasks)
+ {
+ return maxNumConcurrentSubTasks == 1 ? 100L : null;
+ }
Review Comment:
This method intentionally returns `null` to indicate 'use production
default', but the signature/Javadoc doesn’t explicitly convey nullability to
callers and static analysis. Consider marking the return type as nullable
(project annotation, e.g., `@Nullable`) and/or explicitly documenting 'may
return null' in the method Javadoc to make the contract unambiguous.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/PartialCompactionTest.java:
##########
@@ -72,6 +72,7 @@ public class PartialCompactionTest extends
AbstractMultiPhaseParallelIndexingTes
null
);
private static final Interval INTERVAL_TO_INDEX =
Intervals.of("2017-12/P1M");
+ private static final long TASK_STATUS_CHECK_PERIOD_MS = 100L;
Review Comment:
The `100L` poll interval is now duplicated across the base class (default
for serial) and this subclass constant. To avoid drift if the 'short poll'
value ever changes, consider defining a shared constant in the base test class
(e.g., `SHORT_TASK_STATUS_CHECK_PERIOD_MS`) and referencing it here instead of
introducing a second copy.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/PartialCompactionTest.java:
##########
@@ -80,6 +81,18 @@ public PartialCompactionTest()
super(LockGranularity.TIME_CHUNK, DEFAULT_TRANSIENT_TASK_FAILURE_RATE,
DEFAULT_TRANSIENT_API_FAILURE_RATE);
}
+ /**
+ * This test drives several rounds of parallel indexing/compaction (each
with its own
+ * determine-partitions/generate/merge phases) back-to-back, always with
concurrent sub-tasks, so the
+ * base implementation would poll task status at the 1-second production
default and pay up to a full
+ * period per phase transition. None of the assertions here depend on the
poll cadence, so poll quickly.
+ */
+ @Override
+ protected Long getTaskStatusCheckPeriodMs(int maxNumConcurrentSubTasks)
Review Comment:
The override ignores `maxNumConcurrentSubTasks`. If your static analysis
flags unused parameters even in overrides, consider renaming the parameter to
`ignoredMaxNumConcurrentSubTasks` (or adding a targeted suppression) to prevent
future checkstyle/PMD noise and to make the intent explicit.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/PartialCompactionTest.java:
##########
@@ -80,6 +81,18 @@ public PartialCompactionTest()
super(LockGranularity.TIME_CHUNK, DEFAULT_TRANSIENT_TASK_FAILURE_RATE,
DEFAULT_TRANSIENT_API_FAILURE_RATE);
}
+ /**
+ * This test drives several rounds of parallel indexing/compaction (each
with its own
+ * determine-partitions/generate/merge phases) back-to-back, always with
concurrent sub-tasks, so the
+ * base implementation would poll task status at the 1-second production
default and pay up to a full
+ * period per phase transition. None of the assertions here depend on the
poll cadence, so poll quickly.
+ */
+ @Override
+ protected Long getTaskStatusCheckPeriodMs(int maxNumConcurrentSubTasks)
+ {
+ return TASK_STATUS_CHECK_PERIOD_MS;
+ }
Review Comment:
The `100L` poll interval is now duplicated across the base class (default
for serial) and this subclass constant. To avoid drift if the 'short poll'
value ever changes, consider defining a shared constant in the base test class
(e.g., `SHORT_TASK_STATUS_CHECK_PERIOD_MS`) and referencing it here instead of
introducing a second copy.
--
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]
