Copilot commented on code in PR #20306:
URL: https://github.com/apache/druid/pull/20306#discussion_r3969329942
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/PartialCompactionTest.java:
##########
@@ -80,6 +82,32 @@ 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. The base
implementation only
+ * shortens {@link ParallelIndexTuningConfig#getTaskStatusCheckPeriodMs()}
for serial runs
+ * (maxNumConcurrentSubTasks == 1) and otherwise falls back to the 1-second
production default,
+ * which is production-realistic but adds several seconds of avoidable
polling latency per phase
+ * transition in this test. None of the assertions here depend on the poll
cadence, so use a
+ * short interval unconditionally to cut wall-clock time without changing
what is being tested.
+ */
+ @Override
+ protected ParallelIndexTuningConfig newTuningConfig(
+ PartitionsSpec partitionsSpec,
+ int maxNumConcurrentSubTasks,
+ boolean forceGuaranteedRollup
+ )
+ {
+ return TuningConfigBuilder.forParallelIndexTask()
+ .withSplitHintSpec(new
MaxSizeSplitHintSpec(null, 1))
+ .withPartitionsSpec(partitionsSpec)
+ .withForceGuaranteedRollup(forceGuaranteedRollup)
+
.withMaxNumConcurrentSubTasks(maxNumConcurrentSubTasks)
+ .withTaskStatusCheckPeriodMs(100L)
+ .withMaxParseExceptions(5)
+ .build();
Review Comment:
This override duplicates the full base `newTuningConfig` builder chain,
which risks drifting if the base test config changes (e.g., new defaults or
knobs). If feasible, prefer delegating to the superclass implementation and
only overriding `taskStatusCheckPeriodMs` (for example via a builder/copy
method if available), or factor the common builder setup into a shared helper
so only the poll interval differs here.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/PartialCompactionTest.java:
##########
@@ -80,6 +82,32 @@ 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. The base
implementation only
+ * shortens {@link ParallelIndexTuningConfig#getTaskStatusCheckPeriodMs()}
for serial runs
+ * (maxNumConcurrentSubTasks == 1) and otherwise falls back to the 1-second
production default,
+ * which is production-realistic but adds several seconds of avoidable
polling latency per phase
+ * transition in this test. None of the assertions here depend on the poll
cadence, so use a
+ * short interval unconditionally to cut wall-clock time without changing
what is being tested.
+ */
+ @Override
+ protected ParallelIndexTuningConfig newTuningConfig(
+ PartitionsSpec partitionsSpec,
+ int maxNumConcurrentSubTasks,
+ boolean forceGuaranteedRollup
+ )
+ {
+ return TuningConfigBuilder.forParallelIndexTask()
+ .withSplitHintSpec(new
MaxSizeSplitHintSpec(null, 1))
+ .withPartitionsSpec(partitionsSpec)
+ .withForceGuaranteedRollup(forceGuaranteedRollup)
+
.withMaxNumConcurrentSubTasks(maxNumConcurrentSubTasks)
+ .withTaskStatusCheckPeriodMs(100L)
Review Comment:
The `100L` polling period is a behavior-defining value for this test but is
currently a magic number. Consider introducing a named constant (e.g., `private
static final long TASK_STATUS_CHECK_PERIOD_MS = 100L;`) so the intent is
clearer and future tweaks are localized.
--
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]
