[CI] Parallelize unit-test phase with fork-safe port/temp-dir isolation - #19199
Open
xiangfu0 wants to merge 9 commits into
Open
[CI] Parallelize unit-test phase with fork-safe port/temp-dir isolation#19199xiangfu0 wants to merge 9 commits into
xiangfu0 wants to merge 9 commits into
Conversation
The Pinot Unit Tests workflow spends ~83% of its ~63 min wall-clock in a single-threaded `mvn test` phase, using only 1 of the runner's 4 cores. Make surefire forkCount and per-fork heap overridable via properties (default 1 fork / 4g, preserving local/dev behavior). CI opts into 2 forks / 3g so test classes run in parallel JVMs (reuseForks=false keeps one class per JVM, so this is process-level isolation, not TestNG intra-JVM threading). To keep parallel forks collision-free: - ZkStarter offsets its default test port per surefire.forkNumber and uses a UUID (not currentTimeMillis) for the ZK data dir. - ControllerTest offsets its controller/broker/server/minion and configured ZK port bases per fork, and uses UUIDs for its data/temp dirs. A ZK base of 0 (the default) still defers to ZkStarter's fork-aware allocation. - surefire.forkNumber is exposed to forked JVMs via systemPropertyVariables. - JaCoCo writes a per-fork jacoco-<forkNumber>.exec so concurrent forks no longer append to one shared exec file; report-aggregate globs jacoco-*.exec.
…0 min Set 2's serial test phase is ~53 min; at forkCount=2 the parallelized phase (~27 min ideal, more with imperfect balance) plus the ~9.5 min build lands too close to the 40 min target. forkCount=3 with 2500m/fork (3*2500m + the 2g Maven JVM ~= 9.5g, well under the runner's 16g) gives comfortable margin. Test JVMs spend much of their runtime blocked on ZK/Helix/socket startup, so a third fork still pays off on the 4-vCPU runner. Still a single env override (UNIT_TEST_FORK_COUNT) to dial back if CI shows CPU/memory pressure.
Follow-ups from review of the unit-test parallelization: - Move pinot-segment-local's tests to test-set #1 (already built there) so the two shards stay balanced now that set #2's build is ~3x longer than set #1's. - Fix fork-unsafe temp-dir sharing surfaced by parallel forks: DictionariesTest and DictionaryOptimiserTest both derived their index dir from DictionariesTest.class (same path) and wiped it in @BeforeClass, colliding when run concurrently; SegmentLocalFSDirectoryTest derived its dir from another test's class. All three now use a per-run UUID temp dir. - Scope the JaCoCo exec file per lane instead of globally: prepare-agent now writes jacoco${jacoco.exec.suffix}.exec, defaulting to the historical jacoco.exec so the integration lanes' jacoco:report keep working; only the unit-test script sets the suffix to -${surefire.forkNumber} so parallel forks write distinct jacoco-<n>.exec files (aggregated via a jacoco-*.exec glob). - Correct comments: surefire.forkNumber is 1-based (1 even at forkCount=1), and clarify the ZkStarter fork offset is a test-only hook (0 in production).
Two pre-existing tests (SegmentPreProcessorTest's file-mtime assertion and LuceneMutableTextIndexTest's NRT-refresh assertion) fail only when many parallel forks saturate the runner; they pass in isolation and on retry. Add an overridable unit.test.rerun.count (default 0 = unchanged locally) wired to surefire rerunFailingTestsCount, and set it to 2 in the unit-test CI script. Surefire still reports a retry-only pass as flaky, so genuine failures fail the build.
testV3CreateInvertedIndices asserts the no-op second index creation does not rewrite columns.psf by comparing FileTime at nanosecond precision. The 2s sleeps in the test mean a real rewrite moves the mtime by ~2000ms, so nanosecond exactness is unnecessary and flaky: under CPU load (parallel forks) two getLastModifiedTime syscalls can report sub-microsecond jitter for an untouched file. Compare toMillis() instead — still catches any real rewrite.
JaCoCo adds ~30% to the unit-test phase (agent per fork + aggregate report). Add RUN_CODECOVERAGE (default true, so behavior is unchanged) so a caller can set RUN_CODECOVERAGE=false to trade coverage for a faster run when needed. When disabled, the codecoverage profile and the report-aggregate step are skipped.
Replace LuceneMutableTextIndexTest's fixed Thread.sleep(100) after commit() with a bounded poll that waits (up to 30s) until a sentinel query reflects the committed docs. The fixed sleep was too short under CPU load (the async NRT refresh thread may not run in time), which is what made the test flaky under parallel forks. With both load-sensitive flakes now fixed at the root (this and the SegmentPreProcessorTest mtime assertion), set the default unit.test.rerun.count back to 0 so no real failure is ever masked; the knob remains as an escape hatch. Also document that ZkStarter's fork-port offset is deliberately centralized on the no-arg entry point (production-inert; avoids duplicating fork math across several test callers).
…ProcessorTest Even at millisecond granularity the no-op index recreation can leave the columns.psf mtime 1ms later than the prior read under CPU load (filesystem timestamp granularity / metadata flush), so exact equality still flaked. The test's 2s sleeps guarantee a genuine rewrite would move the mtime by ~2000ms, so assert the delta stays under 1s instead — still catches a real rewrite, robust to sub-second jitter.
…tIndexTest
The NRT-refresh barrier polled getDocIds("stream") expecting doc 0, which only
matches under the default StandardAnalyzer. The five custom-analyzer tests use a
KeywordTokenizer that indexes each value as one token, so the term "stream"
never matches and each test spun the full 30s timeout (~150s wasted on the very
class this change speeds up). Poll the regex /.*house.*/ -> doc 1 instead, which
every config indexes and which matches under both analyzers. Full class now runs
in ~7s (was ~150s+).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Unit Tests workflow currently takes ~63 min wall-clock, with ~83% of that spent in a single-threaded
mvn teststep. This PR keeps the existing 2-shard matrix but parallelizes tests within each shard using surefire fork-level parallelism, with fork-safe port/temp-dir isolation so concurrent forks don't collide.All new knobs default to the prior behavior, so local/dev builds are unchanged; only CI opts into parallelism.
What changed
Parallelism knobs (pom.xml, overridable, default = prior behavior)
unit.test.fork.count(default1) → surefireforkCount. WithreuseForks=false, each fork runs one test class in its own JVM, so this is process-level isolation (not TestNG intra-JVM threading) — tests that were unsafe multi-threaded within a single JVM are unaffected. CI sets3.unit.test.fork.heap(default4g) → per-fork-Xms/-Xmx. CI lowers this to2500mso3 forks * 2500m + the Maven JVMstays within the runner's RAM.unit.test.rerun.count(default0) →rerunFailingTestsCount. Left at0; kept as an escape hatch so real failures are never masked.jacoco.exec.suffix(default empty) → per-fork exec file. The unit-test script sets it to-${surefire.forkNumber}so parallel forks each write a distinctjacoco-<n>.execinstead of corrupting one shared file; coverage is then aggregated via ajacoco-*.execglob.surefire.forkNumberexposed as a system property so tests can offset resources per fork.Fork-safe resource isolation
ZkStarter: per-fork ZK test-port offset derived fromsurefire.forkNumber(production-inert — the property is absent outside a surefire fork, so it falls back to the historical default port). ZK data dir now uses a random UUID instead ofSystem.currentTimeMillis()(which collides when two instances start in the same ms).ControllerTest: per-fork controller/ZK port offset; data/temp dirs switched to UUID-based names.Fork-unsafe test fixes (pinot-segment-local)
DictionariesTest/DictionaryOptimiserTest/SegmentLocalFSDirectoryTest: these derived their index dir from another test's class name, so they shared a directory. Switched to per-run UUID dirs under the temp dir.Root-caused flakes (not masked by retry)
SegmentPreProcessorTest: assert the no-op index recreation doesn't move the mtime by more than filesystem timestamp granularity, instead of requiring exact equality (flaky under CPU load).LuceneMutableTextIndexTest: replace the fixedThread.sleep(100)with polling on an analyzer-independent refresh sentinel until the async NRT refresh is visible (up to a generous timeout).CI script (
.pinot_tests_unit.sh)mvn test(modules already built/installed by.pinot_tests_build.sh).UNIT_TEST_FORK_COUNT(3),UNIT_TEST_FORK_HEAP(2500m),UNIT_TEST_RERUN_COUNT(0),RUN_CODECOVERAGE(true).pinot-segment-localtests moved to set add pinot-trace #1 to keep both shards near-equal in wall-clock.jacoco-*.execglob.Backward compatibility
ZkStarter's fork offset is inert outside a surefire fork.jacoco.exec), somvn testlocally behaves exactly as before.Testing
CI Unit Tests workflow (both shards) with parallel forks enabled.