Skip to content

Fix flaky pauseless ingestion integration tests - #19133

Draft
xiangfu0 wants to merge 5 commits into
apache:masterfrom
xiangfu0:xiangfu0/fix-pauseless-segment-completion-timeout
Draft

Fix flaky pauseless ingestion integration tests#19133
xiangfu0 wants to merge 5 commits into
apache:masterfrom
xiangfu0:xiangfu0/fix-pauseless-segment-completion-timeout

Conversation

@xiangfu0

Copy link
Copy Markdown
Contributor

Summary

  • Scope the 10-second segment-completion deadline to the controller failure-injection tests that require it.
  • Configure the short deadline only after the non-pauseless comparison table finishes setup.
  • Keep the short deadline through the explicit validation repair pass, then restore the normal 5-minute deadline.

Root cause

BasePauselessRealtimeIngestionTest globally replaced the controller segment-completion deadline with 10 seconds. That affected ordinary pauseless tests, table rebalance tests, and the non-pauseless comparison table even when no controller fault was injected. On a busy CI runner, normal segment commits exceeded the artificial deadline and were repeatedly repaired, causing otherwise unrelated tests to hit their 20-minute timeout.

How to reproduce

Run the Set 2 integration-test shard containing the pauseless suites under CI load. In the failing job, the basic pauseless test logged 38 segment-completion deadline expirations, the rebalance test logged 14, and the ideal-state failure test logged 2 before five tests timed out. The passing run on the same branch and a passing master run logged no such expirations for those classes.

Testing

  • JDK 25 focused reactor run: ./mvnw -pl pinot-integration-tests -am -Dtest=PauselessRealtimeIngestionIntegrationTest,TableRebalancePauselessIntegrationTest,PauselessRealtimeIngestionIdealStateUpdateFailureTest,PauselessRealtimeIngestionNewSegmentMetadataCreationFailureTest,PauselessRealtimeIngestionCommitEndMetadataFailureTest -Dsurefire.failIfNoSpecifiedTests=false test (6 tests, 0 failures)
  • ./mvnw spotless:apply -pl pinot-integration-tests
  • ./mvnw license:format -pl pinot-integration-tests
  • ./mvnw checkstyle:check -pl pinot-integration-tests
  • ./mvnw license:check -pl pinot-integration-tests
  • JDK 25 warning-enabled reactor test-compile with -Xlint:all

@Jackie-Jiang Jackie-Jiang added the testing Related to tests or test infrastructure label Jul 30, 2026
@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 65.69%. Comparing base (659422d) to head (2a045c4).

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19133      +/-   ##
============================================
+ Coverage     65.68%   65.69%   +0.01%     
  Complexity     1423     1423              
============================================
  Files          3439     3439              
  Lines        218063   218063              
  Branches      34678    34678              
============================================
+ Hits         143232   143258      +26     
+ Misses        63294    63258      -36     
- Partials      11537    11547      +10     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 65.69% <ø> (+0.01%) ⬆️
temurin 65.69% <ø> (+0.01%) ⬆️
unittests 65.69% <ø> (+0.01%) ⬆️
unittests1 57.05% <ø> (+<0.01%) ⬆️
unittests2 37.99% <ø> (+<0.01%) ⬆️

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces flakiness in pauseless real-time ingestion integration tests by scoping the controller’s “max segment completion” timeout override (10s) to only the failure-injection scenarios, preventing normal pauseless and comparison-table ingestion from being forced into repeated deadline-expiry repairs on busy CI runners.

Changes:

  • Apply the 10-second segment-completion timeout only when a failure point is configured (i.e., failure-injection tests).
  • Delay applying the short timeout until after the non-pauseless comparison table has completed setup.
  • Keep the short timeout through the validation/repair pass, then restore the default 5-minute timeout via a finally block.

@xiangfu0
xiangfu0 force-pushed the xiangfu0/fix-pauseless-segment-completion-timeout branch from 220eca6 to 6d10620 Compare July 31, 2026 08:01
@xiangfu0
xiangfu0 requested a review from Copilot July 31, 2026 21:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@xiangfu0
xiangfu0 force-pushed the xiangfu0/fix-pauseless-segment-completion-timeout branch from e5f34a5 to b91ed56 Compare August 1, 2026 08:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasePauselessRealtimeIngestionTest.java:169

  • getFailurePoint() is now explicitly nullable, but injectFailure() / disableFailure() still call enableTestFault(getFailurePoint()) / disableTestFault(getFailurePoint()) and set _failureEnabled even when the failure point is null (e.g. TableRebalancePauselessIntegrationTest). This leaves a null key in the failure-config map and can also flip _failureEnabled to true even though no failure is injected, which can change expectations such as getCountStarResult() in future subclasses. Consider treating null as "no failure" in these helpers.
  protected void injectFailure() {
    PinotLLCRealtimeSegmentManager realtimeSegmentManager = _helixResourceManager.getRealtimeSegmentManager();
    if (realtimeSegmentManager instanceof FailureInjectingPinotLLCRealtimeSegmentManager) {
      ((FailureInjectingPinotLLCRealtimeSegmentManager) realtimeSegmentManager).enableTestFault(getFailurePoint());
    }
    _failureEnabled = true;

@xiangfu0

xiangfu0 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the suppressed review comment on BasePauselessRealtimeIngestionTest: injectFailure() and disableFailure() now return early when getFailurePoint() is null, so no null key is put into the failure-config map and _failureEnabled no longer flips to true for tests that inject no failure (TableRebalancePauselessIntegrationTest). No behavior change today — that class's getCountStarResultWithFailure() already returns DEFAULT_COUNT_STAR_RESULT — but it removes the latent trap for future subclasses. Verified with TableRebalancePauselessIntegrationTest, PauselessRealtimeIngestionIntegrationTest, and the three controller-failure tests (6 tests, 0 failures on JDK 25).

🤖 Addressed by Claude Code

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

Labels

testing Related to tests or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants