Skip to content

Stabilize testReload in PartialUpsertTableRebalanceIntegrationTest - #19125

Open
deepthi912 wants to merge 1 commit into
apache:masterfrom
deepthi912:stabilize-partial-upsert-reload-test
Open

Stabilize testReload in PartialUpsertTableRebalanceIntegrationTest#19125
deepthi912 wants to merge 1 commit into
apache:masterfrom
deepthi912:stabilize-partial-upsert-reload-test

Conversation

@deepthi912

Copy link
Copy Markdown
Collaborator

Summary

PartialUpsertTableRebalanceIntegrationTest#testReload (and its downstream RocksDB subclass) can flake with expected [ONLINE] but found [OFFLINE] at verifySegmentAssignment line 304. The failure is a state-check-too-early issue in the test, not a product bug.

Why

waitForReloadToComplete polls the reload job status API, which only records per-segment reload completion. For a partial-upsert reload in PROTECTED consistency mode, the CONSUMING segment is force-committed, the controller atomically flips it CONSUMING → ONLINE and creates a new CONSUMING segment. But:

  1. The reload job's totalSegmentCount is a snapshot at job start — it doesn't include the new CONSUMING segments created by force-commit.
  2. waitForAllDocsLoaded uses broker SELECT COUNT(*) which routes via ExternalView; it cannot detect that an IdealState replica flipped OFFLINE while another still serves.
  3. If a server-side error (e.g., consumer init failure on the new segment) fires postStopConsumedMsg, PinotLLCRealtimeSegmentManager.segmentStoppedConsuming marks the segment OFFLINE in IdealState; RealtimeSegmentValidationManager then creates a repair segment. IdealState now has an older segment with seq < maxSeq in OFFLINE state — which is exactly what the assertion sees.

None of the existing waits look at IdealState directly.

Changes (test-only, no product code)

File: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

  1. New waitForClusterStateSettled(table, expectedSegments, stableSamples, timeoutMs) that requires all of:
    • IdealState has expectedSegments entries.
    • No segment has any instance in OFFLINE state.
    • ExternalView.mapFields.equals(IdealState.mapFields).
    • Holds for stableSamples consecutive polls (5 samples at 200ms → ~1s of stability).
  2. testReload calls it between waitForAllDocsLoaded and verifyIdealState.
  3. verifySegmentAssignment assertions now include segment name, instance, maxSequenceNumber, and the full assignment map. If a real server-side stall causes OFFLINE (not a race), the CI log will surface which segment on which instance — not a bare expected [ONLINE] but found [OFFLINE].

Test plan

  • ./mvnw spotless:apply checkstyle:check -pl pinot-integration-tests — clean
  • ./mvnw -pl pinot-integration-tests -am test -Dtest=PartialUpsertTableRebalanceIntegrationTest#testReload — passes (23s)
  • CI: rerun the flaky suite; expect no regression and cleaner failure messages if a real stall surfaces

waitForReloadToComplete only tracks per-segment reload completion, not
IdealState settlement. For partial-upsert reload the CONSUMING segment
is force-committed and a new CONSUMING segment is created; the reload
job can report done while IdealState still holds a transient OFFLINE or
ExternalView still lags.

Add waitForClusterStateSettled that requires expected segment count, no
OFFLINE anywhere, and ExternalView == IdealState, sustained across
5 samples (~1s). Also add descriptive failure messages to
verifySegmentAssignment so a real stall surfaces the segment name,
instance, and full assignment map instead of a bare
"expected [ONLINE] but found [OFFLINE]".
@codecov-commenter

codecov-commenter commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.89%. Comparing base (b69f5fe) to head (f5980d0).
⚠️ Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (b69f5fe) and HEAD (f5980d0). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (b69f5fe) HEAD (f5980d0)
unittests1 1 0
unittests 2 1
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #19125       +/-   ##
=============================================
- Coverage     65.56%   37.89%   -27.67%     
- Complexity     1417     1422        +5     
=============================================
  Files          3432     3432               
  Lines        218110   218110               
  Branches      34661    34661               
=============================================
- Hits         143009    82661    -60348     
- Misses        63533   127852    +64319     
+ Partials      11568     7597     -3971     
Flag Coverage Δ
custom-integration1 100.00% <ø> (?)
integration 100.00% <ø> (+100.00%) ⬆️
integration1 100.00% <ø> (?)
integration2 0.00% <ø> (ø)
java-25 37.89% <ø> (-27.67%) ⬇️
temurin 37.89% <ø> (-27.67%) ⬇️
unittests 37.89% <ø> (-27.68%) ⬇️
unittests1 ?
unittests2 37.89% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants