Jackie-Jiang opened a new pull request, #19356: URL: https://github.com/apache/pinot/pull/19356
## Summary Two changes to `BrokerRoutingManagerConcurrencyTest`, both replacing scheduling-dependent constructs with deterministic ones. ### Fix flaky `testConcurrentGlobalWriteLockMethods` Observed CI failure: `Event at position 1 should be an end event expected [true] but found [false]`. The test ran three global-write-lock methods on three threads and asserted the recorded `_start`/`_end` events form contiguous pairs. But the markers were recorded in the caller threads, while the methods acquire `_globalLock.writeLock()` inside `BaseBrokerRoutingManager` — so the events observed invocation order, not critical-section boundaries. A thread descheduled between recording its `_start` marker and acquiring the (fair) lock legally interleaves the events, and the `Thread.sleep(10/20)` "encouragements" cannot prevent that on a loaded host. Rewritten to verify the actual contract: the test holds the global write lock itself, invokes `processClusterChange(INSTANCE_CONFIG)`, `includeServerToRouting` and `excludeServerFromRouting` concurrently, asserts none of them can complete while the lock is held (proving each gates on the global write lock, i.e. they are mutually exclusive), then releases the lock and asserts all three complete (no deadlock) with the routing state intact. The assertion polarity makes it robust: correct code blocks indefinitely, so the bounded negative check cannot flake on a slow host, while code that skips the lock completes within the window and is caught. ### Strengthen the TimeBoundaryManager assertions in the multi-physical-table tests `testConcurrentMultiPhysicalTableLogicalOperations` and `testConcurrentMultiPhysicalTableLogicalOperationsWithRealtimeBuild` asserted the time boundary manager state only after re-running `buildRouting` on the asserted tables. A rebuild recomputes the manager purely from the current sibling-routing state, so the assertions could never observe what the concurrent phase produced — the "CRITICAL VERIFICATION" was vacuous. The assertions now run before the rebuilds, and the logical-build -> logical-remove ordering they rely on is enforced by a latch instead of `Thread.sleep(20/25)`, keeping the asserted final state deterministic under any scheduling of the remaining racing operations. Also includes a cleanup pass over the file: static-import the TestNG asserts and drop the unused `Future` captures. -- 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]
