Skip to content

[fix][test] Fix flaky test testShadowWrites #22745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merlimat merged 1 commit into from
May 23, 2024
Merged

[fix][test] Fix flaky test testShadowWrites #22745

merlimat merged 1 commit into from
May 23, 2024

Conversation

@RobertIndie
Copy link
Member

@RobertIndie RobertIndie commented May 20, 2024

Fixes #22345

Motivation

Root cause analysis

Actually, the ledger will roll over here because it will fill up the current ledger:

pulsar/managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImplTest.java

Line 77 in 6372b9c

Position newPos = sourceML.addEntry(data);

Therefore, the source ML will update the managed ledger info. And the shadowML will get the updated MLInfo and update the LCE. So the assertion here is incorrect because the LCE will be updated:

pulsar/managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImplTest.java

Lines 79 to 81 in 6372b9c

// The state should not be the same.
log.info("Source.LCE={},Shadow.LCE={}", sourceML.lastConfirmedEntry, shadowML.lastConfirmedEntry);
assertNotEquals(sourceML.lastConfirmedEntry, shadowML.lastConfirmedEntry);

The process of updating MLInfo from sourceML to shadowML is asynchronous. This causes the test to be unreliable.

Regarding the change for this:

pulsar/managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImplTest.java

Lines 151 to 154 in 6372b9c

Awaitility.await().untilAsserted(() -> {
assertEquals(shadowML.ledgers.size(), 6);
assertEquals(shadowML.currentLedgerEntries, 0);
});

We have removed one entry addition operation in the test. Therefore, the final count for the ledger size will be one less.

Modifications

  • Remove incorrect assertions from the tests
  • Improve the test by using Awaitility
  • Move the tests out of flaky group

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@RobertIndie RobertIndie added type/bug The PR fixed a bug or issue reported a bug type/flaky-tests labels May 20, 2024
@RobertIndie RobertIndie added this to the 3.4.0 milestone May 20, 2024
@RobertIndie RobertIndie self-assigned this May 20, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 20, 2024
@RobertIndie RobertIndie requested a review from Jason918 May 22, 2024 08:05
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.22%. Comparing base (bbc6224) to head (bc6c276).
⚠️ Report is 1253 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22745      +/-   ##
============================================
- Coverage     73.57%   73.22%   -0.35%     
+ Complexity    32624     2413   -30211     
============================================
  Files          1877     1889      +12     
  Lines        139502   141419    +1917     
  Branches      15299    15518     +219     
============================================
+ Hits         102638   103558     +920     
- Misses        28908    29859     +951     
- Partials       7956     8002      +46
Flag Coverage Δ
inttests 27.48% <ø> (+2.90%) ⬆️
systests 24.72% <ø> (+0.39%) ⬆️
unittests 72.23% <ø> (-0.61%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 365 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@merlimat merlimat merged commit 99bff72 into apache:master May 23, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@RobertIndie @hanmz
[fix][test] Fix flaky test testShadowWrites (apache#22745)
3df76cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

@Jason918 Jason918 Awaiting requested review from Jason918

Assignees

@RobertIndie RobertIndie

Labels

doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug type/flaky-tests

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

Flaky-test: ShadowManagedLedgerImplTest.testShadowWrites

4 participants

@RobertIndie @codecov-commenter @BewareMyPower @merlimat