xiangfu0 opened a new pull request, #19195: URL: https://github.com/apache/pinot/pull/19195
Cuts redundant cluster startups from the ingestion integration tests. Three independent commits, each revertable on its own. ## Why Measured from the surefire artifacts of two successful `master` runs ([31223104137](https://github.com/apache/pinot/actions/runs/31223104137), [31210800735](https://github.com/apache/pinot/actions/runs/31210800735)), cross-checked for stability (per-class rankings agree within ~15%). Each artifact carries two report trees, and subtracting one from the other isolates setup cost: `surefire-reports/TEST-*.xml` is JVM wall-clock per class (includes `@BeforeClass`), while `surefire-reports/junitreports/TEST-*.xml` is the TestNG sum of `@Test` method times. - **129.2 min** of integration-test wall-clock across both jobs. - **83.5 min (65%) is ingestion** — 50 classes. - **52.2 min of that (63%) is `@BeforeClass` cluster setup, not assertions.** `forkCount=1, reuseForks=false` gives every class its own JVM plus a fresh ZK + controller + broker + server + Kafka. The extreme cases spend ~90-99% of their runtime standing that up: ``` PurgeMetadataPushMinionClusterIntegrationTest 118.7s wall / 106.6s setup / 1 test PauselessRealtimeIngestionIntegrationTest 108.0s wall / 106.9s setup / 1 test PinotLLCRealtimeSegmentManagerIntegrationTest 93.3s wall / 92.3s setup / 1 test ``` `custom-cluster-integration-test-suite` already demonstrates the alternative — one cluster in `@BeforeSuite`, **47 classes / 775 methods in 472.7 s**. ## Changes **1. `LLCRealtimeKafka3ClusterIntegrationTest` deleted — no coverage lost.** It extends `LLCRealtimeClusterIntegrationTest` and overrides the stream consumer factory, but the parent *already* uses `org.apache.pinot.plugin.stream.kafka30.KafkaConsumerFactory`. The two `Excepting*ConsumerFactory` inner classes are byte-identical after normalizing the class-name infix (3230 chars, zero differences). It re-ran all 19 inherited tests against the same plugin with the same injected failures. **2. `LLCRealtimeKafka4ClusterIntegrationTest` scoped to its plugin — 270.5s → 73s.** Kafka 4.x is a genuinely different plugin, so this one stays, but it no longer inherits the query suite. Of the 151 s it spent in test methods, ~140 s was `testQueriesFromQueryFile` / `testReload` / `testGeneratedQueries` / `testHardcodedQueries` / `testAddRemoveDictionaryAndInvertedIndex` — query-engine tests that cannot behave differently per consumer factory, and that still run in `LLCRealtimeClusterIntegrationTest`. It now covers what is specific to the plugin: end-to-end consumption, recovery from exceptions thrown during consumer creation and during fetch, tombstone handling, and the segment flush-size contract. It also drops the segment build-and-upload setup, which existed for the parent's upload-path coverage. `injectTombstones()` and the off-heap allocation server setting are overridden explicitly, so re-parenting to `BaseClusterIntegrationTest` does not silently change what the Kafka 4.x consumer is exercised against. This is the only integration test that loads `pinot-kafka-4.0`, so losing the tombstone flag by inheritance would have left that consumer with no null-payload coverage at all. **3. `PurgeMetadataPushMinionClusterIntegrationTest` folded into its parent — 277.7s → 117s.** It stood up a whole cluster to run one test, having disabled five inherited ones with `@Test(enabled = false)` — which suppresses assertions but not setup. It is now a seventh offline table plus one extra test method on `PurgeMinionClusterIntegrationTest`, sharing the cluster that class already builds; the first-run purge flow is extracted into `runFirstRunPurge(tableName)` so both push modes reuse it. The new table is inserted *before* `PURGE_ALL_RECORDS_TABLE` so the trailing `tableConfig`/`schema` that `buildSegmentsFromAvro` consumes is unchanged. **4. `TableRebalancePauselessIntegrationTest` stops building an unused table — 177.6s → 110s.** It inherited `setupNonPauselessTable()` from `BasePauselessRealtimeIngestionTest`, which exists so the failure-injection tests can compare segment ZK metadata across two tables. This class never compares metadata across tables, so that second 48-segment ingestion pass was pure setup cost. ## Validation Every touched class run locally on JDK 25: | Class | Before (CI) | After (local) | Result | |---|---:|---:|---| | `LLCRealtimeKafka4ClusterIntegrationTest` | 270.5s | **73s** | 2/2 pass | | `PurgeMinionClusterIntegrationTest` (+ folded metadata-push) | 159.0s + 118.7s | **117s** | 7/7 pass | | `TableRebalancePauselessIntegrationTest` | 177.6s | **110s** | 2/2 pass | | `LLCRealtimeKafka3ClusterIntegrationTest` | 273.6s | **deleted** | — | Roughly **10 minutes** off total integration-test work. Because the two CI jobs run in parallel and the split is a first-letter glob (`A*`–`N*` / `O*`–`Z*`), the wall-clock saving depends on which job each class lands in — see the follow-ups below. ## Tried and abandoned I also attempted to merge the four pauseless failure-injection classes (`Base…` plus the ideal-state, new-segment-metadata and commit-end-metadata variants, 471s combined) into one shared-cluster class. It reproduced across three runs and I could not make it pass without weakening assertions, so it is **not** in this PR: | Approach | Time | Result | |---|---:|---| | single validation-manager call | 335s | 2 of 4 fail | | looped validation manager | 1926s | 3 of 4 fail | | reverted + 300s timeout + 3600s periodic delay | 706s | 2 of 4 fail | `testFailureBeforeIdealStateUpdate` and `testFailureBeforeNewSegmentMetadataCreation` — exactly the two scenarios that stall at 2 of 48 segments — recover their doc count and ideal state, but leave segments `COMMITTING` with a null download URL indefinitely. A 300s timeout ruled out slow catch-up. Nearly all their segments commit *after* the single manual `RealtimeSegmentValidationManager` pass, and the deep-store retry upload never revisits them. Sharing a controller across these scenarios changes recovery dynamics in a way that wants a pauseless-ingestion owner's judgement. Worth noting the ceiling is lower than it first looks anyway: each scenario has to build its own 48-segment table *while its own fault is active*, so only the cluster start, Kafka push and reference table are shareable — about 40%, not the ~55% a naive setup-cost reading suggests. ## Follow-ups (not in this PR) - **`PauselessRealtimeIngestionIntegrationTest` builds a 48-segment comparison table it never reads.** Its only test calls `testBasicSegmentAssignment()`, which never touches `DEFAULT_TABLE_NAME_2`. The same fix as change 4, and independent of the merge I abandoned. A `useNonPauselessComparisonTable()` hook on the base would cover both. - **Shared helper for the LLC recovery wait.** `runValidationJob`/`isOffline` are now duplicated between `LLCRealtimeClusterIntegrationTest` and the Kafka 4.x test (down from three copies, but still two). Extending `BaseRealtimeClusterIntegrationTest` would drag the query suite back in, so the fix is a shared static helper, not a base class. - **`waitForTaskToComplete()` polls task states cluster-wide**, so one stuck purge task makes every subsequent method burn the full 600s. `PinotHelixTaskResourceManager` already exposes `getTaskStatesByTable`. - **The test-set split is a first-letter glob.** A new `P*Test` lands in set 2 regardless of cost. An explicit runtime-balanced list would let savings like these actually shorten the build. - **23 test classes never execute in CI.** The surefire include patterns (`pinot-integration-tests/pom.xml:91-137`) are `**/…/tests/<Letter>*Test.java`, which cannot match a further subdirectory — so `logicaltable/`, `realtime/ingestion/` (Kinesis, Kafka partition changes), `multicluster/`, `udf/` and `legacy/` are silently skipped. `CancelQueryIntegrationTests` is also missed because it ends in `Tests`. - **`TestUtils.waitForCondition`'s exception-tolerant overload spins.** `Thread.sleep(checkIntervalMs)` sits inside the `try` after the predicate, so a throwing predicate skips the sleep entirely — I hit 1.6M iterations in 600s while writing the abandoned change. Affects every caller whose predicate can throw. -- 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]
