You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "FrankChen021 (via GitHub)" <gi...@apache.org> on 2026/08/03 12:27:49 UTC
[PR] test(processing): reduce IndexIOTest Surefire overhead (druid)
FrankChen021 opened a new pull request, #19859:
URL: https://github.com/apache/druid/pull/19859
### Description
`IndexIOTest` used JUnit 5 `@ParameterizedClass`, which caused each of its 5,925 method-source tuples to be reported as a separate Surefire test set. The resulting stream of `Running` and `Tests run: 1` events can overwhelm the Surefire fork communication channel; the failing master run timed out after 60 minutes while the fork was blocked in that channel.
This patch:
- converts `IndexIOTest` to one `@ParameterizedTest` while preserving all generated cases and assertions;
- creates fresh incremental indexes for each invocation; and
- enables phrased parameterized testcase names in Surefire XML reports, for example `...[1] schema=[{}], rows=[{}], dimensions=null`.
The change does not alter Druid runtime behavior or reduce test coverage.
### Performance comparison
Measured locally on the same checkout with JDK 25 and the same Maven command:
```text
mvn -B -pl processing -Dtest=IndexIOTest \
-Dsurefire.failIfNoSpecifiedTests=false \
-Dweb.console.skip=true -P skip-static-checks test
```
| Configuration | Tests | Surefire test time | Wall-clock time |
| --- | ---: | ---: | ---: |
| `@ParameterizedClass` baseline | 5,925 passed | 189.5 s | 3:31.95 |
| `@ParameterizedTest` + phrased XML reporting | 5,925 passed | 3.459 s | 22.633 s |
The method-level parameterization reduced the local Surefire test phase by roughly 55x. Enabling phrased XML reporting added no material runtime cost; the report contains all 5,925 named testcase entries and is approximately 2 MB.
### Verification
- Ran the complete `IndexIOTest` parameter stream: 5,925 passed.
- Confirmed the Surefire XML report contains 5,925 testcase entries with the parameterized display names.
- Ran `git diff --check`.
- Self-reviewed the complete diff against `master`.
<hr>
##### Key changed/added classes in this PR
* `processing/src/test/java/org/apache/druid/segment/IndexIOTest.java`
* Surefire configuration in `pom.xml`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added or modified existing unit tests to preserve coverage.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(processing): stabilize flaky IndexIOTest execution (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 merged PR #19859:
URL: https://github.com/apache/druid/pull/19859
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] test(processing): stabilize flaky IndexIOTest execution (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19859:
URL: https://github.com/apache/druid/pull/19859#discussion_r3703956089
##########
processing/src/test/java/org/apache/druid/segment/IndexIOTest.java:
##########
@@ -236,48 +231,35 @@ public static List<Map> filterNullValues(List<Map<String, Object>> mapList)
return Lists.transform(mapList, (Function<Map, Map>) input -> Maps.filterValues(input, Objects::nonNull));
}
- @Parameter(0)
- public Collection<Map<String, Object>> events1;
-
- @Parameter(1)
- public Collection<Map<String, Object>> events2;
-
- @Parameter(2)
- public Class<? extends Exception> exception;
-
-
- final IncrementalIndex incrementalIndex1 = new OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- final IncrementalIndex incrementalIndex2 = new OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- IndexableAdapter adapter1;
- IndexableAdapter adapter2;
+ private static IncrementalIndex createIncrementalIndex()
+ {
+ return new OnheapIncrementalIndex.Builder()
+ .setIndexSchema(
+ new IncrementalIndexSchema.Builder()
+ .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
+ .withMetrics(new CountAggregatorFactory("count"))
+ .withDimensionsSpec(
+ new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
+ )
+ .build()
+ )
+ .setMaxRowCount(1000000)
+ .build();
+ }
- @BeforeEach
- public void setUp()
+ @ParameterizedTest(
+ name = "[{index}] schema={0}, rows={1}, dimensions={2}"
+ )
+ @MethodSource("constructionFeeder")
+ public void testRowValidatorEquals(
+ Collection<Map<String, Object>> events1,
+ Collection<Map<String, Object>> events2,
+ Class<? extends Exception> exception
+ ) throws Exception
Review Comment:
The parameterized display name labels don’t match the actual parameters: `{0}`/`{1}` are `events1`/`events2`, and `{2}` is an exception class (not `dimensions`). This makes reports misleading; update the name template to reflect the real argument meanings (or use `Named` arguments and `{argumentsWithNames}` to keep names accurate and concise).
##########
processing/src/test/java/org/apache/druid/segment/IndexIOTest.java:
##########
@@ -236,48 +231,35 @@ public static List<Map> filterNullValues(List<Map<String, Object>> mapList)
return Lists.transform(mapList, (Function<Map, Map>) input -> Maps.filterValues(input, Objects::nonNull));
}
- @Parameter(0)
- public Collection<Map<String, Object>> events1;
-
- @Parameter(1)
- public Collection<Map<String, Object>> events2;
-
- @Parameter(2)
- public Class<? extends Exception> exception;
-
-
- final IncrementalIndex incrementalIndex1 = new OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- final IncrementalIndex incrementalIndex2 = new OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- IndexableAdapter adapter1;
- IndexableAdapter adapter2;
+ private static IncrementalIndex createIncrementalIndex()
+ {
+ return new OnheapIncrementalIndex.Builder()
+ .setIndexSchema(
+ new IncrementalIndexSchema.Builder()
+ .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
+ .withMetrics(new CountAggregatorFactory("count"))
+ .withDimensionsSpec(
+ new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
+ )
+ .build()
+ )
+ .setMaxRowCount(1000000)
+ .build();
+ }
- @BeforeEach
- public void setUp()
+ @ParameterizedTest(
+ name = "[{index}] schema={0}, rows={1}, dimensions={2}"
+ )
Review Comment:
Including full `Collection<Map<...>>` values in the testcase name can produce very large and potentially non-deterministic strings (e.g., map iteration order), inflating XML and making reports noisy/unreliable. Prefer naming based on stable, compact identifiers (like case ids, sizes, or `Named.of(...)` summaries) rather than dumping entire argument payloads.
##########
processing/src/test/java/org/apache/druid/segment/IndexIOTest.java:
##########
@@ -236,48 +231,35 @@ public static List<Map> filterNullValues(List<Map<String, Object>> mapList)
return Lists.transform(mapList, (Function<Map, Map>) input -> Maps.filterValues(input, Objects::nonNull));
}
- @Parameter(0)
- public Collection<Map<String, Object>> events1;
-
- @Parameter(1)
- public Collection<Map<String, Object>> events2;
-
- @Parameter(2)
- public Class<? extends Exception> exception;
-
-
- final IncrementalIndex incrementalIndex1 = new OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- final IncrementalIndex incrementalIndex2 = new OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- IndexableAdapter adapter1;
- IndexableAdapter adapter2;
+ private static IncrementalIndex createIncrementalIndex()
+ {
+ return new OnheapIncrementalIndex.Builder()
+ .setIndexSchema(
+ new IncrementalIndexSchema.Builder()
+ .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
+ .withMetrics(new CountAggregatorFactory("count"))
+ .withDimensionsSpec(
+ new DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
+ )
+ .build()
+ )
+ .setMaxRowCount(1000000)
Review Comment:
The literal `1000000` is a new magic number in a helper used by the test. Consider extracting it to a `private static final int MAX_ROW_COUNT = ...` (or reusing an existing constant in the file, if any) to make intent explicit and avoid future drift if the value needs to change.
##########
pom.xml:
##########
@@ -2062,6 +2062,9 @@
<trimStackTrace>${surefire.trimStackTrace}</trimStackTrace>
<!-- our tests are very verbose, let's keep the volume down -->
<redirectTestOutputToFile>true</redirectTestOutputToFile>
+ <statelessTestsetReporter implementation="org.apache.maven.plugin.surefire.extensions.junit5.JUnit5Xml30StatelessReporter">
+ <usePhrasedTestCaseMethodName>true</usePhrasedTestCaseMethodName>
+ </statelessTestsetReporter>
Review Comment:
This changes Surefire XML reporting behavior globally (and may increase XML verbosity/size for all parameterized tests across the build). If the motivation is specific to `IndexIOTest`/`processing`, consider scoping this to the relevant module/profile or guarding it behind a property so other modules and CI consumers aren’t forced into the new reporting mode.
##########
pom.xml:
##########
@@ -2062,6 +2062,9 @@
<trimStackTrace>${surefire.trimStackTrace}</trimStackTrace>
<!-- our tests are very verbose, let's keep the volume down -->
<redirectTestOutputToFile>true</redirectTestOutputToFile>
+ <statelessTestsetReporter implementation="org.apache.maven.plugin.surefire.extensions.junit5.JUnit5Xml30StatelessReporter">
+ <usePhrasedTestCaseMethodName>true</usePhrasedTestCaseMethodName>
+ </statelessTestsetReporter>
Review Comment:
The configuration references a concrete reporter implementation class; the build will fail if the configured Maven Surefire plugin version on the plugin classpath doesn’t include this extension. It would be safer to (a) ensure the Surefire plugin version is explicitly pinned to a version known to provide `JUnit5Xml30StatelessReporter`, and/or (b) add a short comment in the pom noting the minimum required Surefire version for this config.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org