Skip to content
New issue

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

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

Already on GitHub? # to your account

[fix][test] Flaky-test: ManagedLedgerTest.testTimestampOnWorkingLedger #22600

Merged
merged 2 commits into from
Apr 27, 2024

Conversation

shibd
Copy link
Member

@shibd shibd commented Apr 26, 2024

Motivation

#22430

After #22034, When the ledger is closed due to full entry, will trigger create a new ledger, However, since the creation of the ledger is asynchronous, sometimes the last ledger is not created before the function returns.

// Use the executor here is to avoid use the Zookeeper thread to create the ledger which will lead
// to deadlock at the zookeeper client, details to see https://github.com/apache/pulsar/issues/13736
this.executor.execute(() ->
asyncCreateLedger(bookKeeper, config, digestType, this, Collections.emptyMap()));

For this test: ml.close(); is no make sure that the currentLedger switch successful, and there are two possible case:

  • Case1: [Ledger01{entry1}], [Ledger02{entry1}], [Ledger03{}]
  • Case2: [Ledger01{entry1}], [Ledger02{entry1}]

Case 1 will cause the test to fail.

Modifications

When ml.close to reopen a new ManagerLedger that will make sure into Case1, and then to assert timestamp of Ledger02 than 0.

Verifying this change

  • Run this test locally 1000 times always successful.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.92%. Comparing base (bbc6224) to head (d8cdd6e).
Report is 193 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22600      +/-   ##
============================================
+ Coverage     73.57%   73.92%   +0.34%     
+ Complexity    32624    32606      -18     
============================================
  Files          1877     1885       +8     
  Lines        139502   140524    +1022     
  Branches      15299    15452     +153     
============================================
+ Hits         102638   103877    +1239     
+ Misses        28908    28600     -308     
- Partials       7956     8047      +91     
Flag Coverage Δ
inttests 26.99% <ø> (+2.41%) ⬆️
systests 24.39% <ø> (+0.07%) ⬆️
unittests 73.21% <ø> (+0.37%) ⬆️

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

see 250 files with indirect coverage changes

@Technoboy- Technoboy- added this to the 3.3.0 milestone Apr 26, 2024
@shibd shibd merged commit 5d9ccd4 into apache:master Apr 27, 2024
48 of 49 checks passed
shibd added a commit that referenced this pull request Apr 27, 2024
shibd added a commit that referenced this pull request Apr 27, 2024
shibd added a commit that referenced this pull request Apr 27, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request May 13, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request May 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants