xiangfu0 opened a new pull request, #19652: URL: https://github.com/apache/pinot/pull/19652
> Stacked on #19651 — please merge that first. This PR targets its branch, so review the second commit only; I will retarget to `master` once #19651 lands. ## Problem The lane include patterns are single-level globs such as `org/apache/pinot/integration/tests/L*Test.java`, and a `*` never crosses a directory boundary, so **classes in subpackages were never matched**. Only `tpch/` and `server/realtime/` (explicit entries), `custom/` (CustomClusterSuite) and the two Kinesis classes (KinesisSuite) ever ran. Never selected by any lane: `logicaltable/` (9 classes), `multicluster/` (2), `legacy/` (1), `udf/` (1), `realtime/ingestion/KafkaIncreaseDecreasePartitionsIntegrationTest`, and `CancelQueryIntegrationTests` — the last one missed because its name ends in `Tests`, which no A-Z prefix matches. ## What now runs Balanced against the last healthy lane runtimes (set-1 lane-a 2524s, lane-b 2555s, set-2 lane-a 2055s, lane-b 2231s): | lane | added | | --- | --- | | set-1 lane-b | `legacy/` | | set-2 lane-a | `LogicalTableSuite`, `KafkaPartitionSubsetChaosIntegrationTest` | | set-2 lane-b | `multicluster/` | `logicaltable/` needs a **focused suite execution** rather than a lane include: `BaseLogicalTableIntegrationTest` starts its cluster from `@BeforeSuite`, so its subclasses share that cluster only when they run as one suite. `LogicalTableSuite` selects the whole package, the way `CustomClusterSuite` does for `custom/`, so a class added later cannot be silently skipped. It excludes `KafkaPartitionSubsetChaosIntegrationTest`, which lives in the package but does not extend that base and starts its own cluster from `@BeforeClass`. Inside the suite that cluster would sit alongside the shared one for its whole duration, and it timed out there while passing on its own. A lane selects it directly instead. ## Failures this exposed, all fixed here - **`NotUdf` was broken in main code.** It looked up `LogicalFunctions.not(boolean)`, but #17189 changed the signature to `not(Boolean)`. The constructor threw `NoSuchMethodException`, so every `ServiceLoader.load(Udf.class)` failed with `ServiceConfigurationError` and `UdfTest` could not run at all. Only the UDF test framework loads that SPI, so no query path was affected. - **`LogicalTableWithTwoRealtimeTableIntegrationTest` could not create its tables.** `getKafkaTopic()` defaults to the class simple name, so every subclass has its own topic, but `@BeforeSuite` runs on one instance and creates only that instance's topic. This class's topic was left to be auto-created with a single partition, so the controller rejected its second table for pinning `stream.kafka.partition.ids=1`. Each class now creates its own topic with its own partition count before its table configs are validated. - **The suite failed in teardown even with every test passing.** `tearDown()` purges the shared cluster through `cleanup()`, which reads Helix and the property store directly, but only the `@BeforeSuite` instance held those handles. The resulting `NullPointerException` also replaced the message `cleanup()` was about to report, so it pointed at a null field rather than at the cleanup that actually failed. Non-owner subclasses now inherit those handles. - **`testLogicalTableWithEmptyOfflineTable` had a stale assertion.** It still expected `numServersQueried == 1` for the multi-stage engine; #18538 made the multi-stage broker short-circuit when all leaf stages are empty, so both engines now report 0. - **Three tests raced config propagation.** `testQueryTimeOut`, `testMaxQueryResponseSizeTableConfig` and `testDisableGroovyQueryTableConfigOverride` each applied a query config through the controller and then asserted on the very next query. `updateLogicalTableConfig` is asynchronous — brokers observe the change through a ZooKeeper property store listener — so the assertion raced propagation. They now wait for the new behavior through a shared helper. `testQueryTimeOut` also accepts every stage's timeout code, which lets `LogicalTableWithTwoRealtimeTableIntegrationTest` drop its own copy of the test instead of maintaining a list that omitted `EXECUTION_TIMEOUT`. ## Verification `LogicalTableSuite` run locally on this branch: **146 tests, 0 failures, 0 errors**. That includes `testProtoSegmentListPreservesLogicalTableResults`, added to this package by #19568, which had also never executed in CI. `multicluster/` and `legacy/` were run per class: `MultiClusterIntegrationTest` 20 tests, `SameTableNameMultiClusterIntegrationTest` 20, `LegacyRawValueInvertedIndexMigrationIntegrationTest` 10 — all green. `KafkaPartitionSubsetChaosIntegrationTest` passes standalone (5 tests). Static analysis of the patterns confirms no class is selected by two lanes and nothing in the newly added packages is missed. ## Deliberately still not selected Each has the reason recorded next to the pattern. They fail for their own pre-existing reasons rather than anything in this change, so enabling them would just turn CI red: - **`UdfTest`** — its snapshots under `src/test/resources/udf-test-results` went stale while the class was unrunnable. Most of the drift is additive (functions added to the registry, `and` gaining a scalar from #17189, `abs` rendering BigDecimal as `"3.0"` rather than `"3"` under `BIG_DECIMAL_AS_DOUBLE` equivalence). But refreshing them would also record that transform initialization errors now surface as a generic `"Operator execution error"` instead of naming the cause, e.g. `"Caught exception while initializing transform function: equals: null"`. That looks like a diagnosability regression and wants a deliberate decision before it is baked into the snapshots. Note when reading its output that its two tests call `DiffUtils.diff` with opposite argument order, so the diffs read in opposite directions. - **`CancelQueryIntegrationTests`** — `testCancelByClientQueryId` observes no `QueryCancellationError` on either engine. - **`realtime/ingestion/KafkaIncreaseDecreasePartitionsIntegrationTest`** — `POST /tables` hangs until the 60s client timeout when the test adds its second realtime table. Reproduced twice. Happy to file issues for these three if that is preferred over the inline notes. ## Cleanup `.pinot_tests_integration.sh`, `.pinot_tests_custom_integration.sh` and `.pinot_tests_kinesis_integration.sh` are referenced by no workflow, and the kinesis one gates on `RUN_TEST_SET==2` although `KinesisSuite` runs in set-1 lane-a. They are removed together with the `integration-tests-set-1` and `integration-tests-set-2` profiles, whose only consumer they were and whose duplicated include lists are what let the lanes drift unnoticed. The four lane profiles are now the single source of truth. ## Also worth noting Three classes excluded by #19190 with no justifying comment still run nowhere, and all three did run in set 2 before it: `PinotLLCRealtimeSegmentManagerIntegrationTest`, `QueryThreadContextIntegrationTest`, `SpoolIntegrationTest`. Left alone here since they are not subpackage classes, but they are worth a look. -- 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]
