Skip to content
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.

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

[fix][test] flaky test ManagedLedgerBkTest.asyncMarkDeleteAndClose #18362

Merged

Conversation

poorbarcode
Copy link
Contributor

Fixes #16729

Motivation

same as #18310

ManagedCursorImpl has a known problem: when the cursor.close and the cursor.switchLedger are concurrent executing, a bad version exception is thrown. see:

if (e instanceof MetaStoreException.BadVersionException) {
log.warn("[{}] Failed to update cursor metadata for {} due to version conflict {}",
ledger.name, name, e.getMessage());
// it means previous owner of the ml might have updated the version incorrectly. So, check
// the ownership and refresh the version again.
if (ledger.mlOwnershipChecker != null && ledger.mlOwnershipChecker.get()) {
ledger.getStore().asyncGetCursorInfo(ledger.getName(), name,
new MetaStoreCallback<ManagedCursorInfo>() {
@Override
public void operationComplete(ManagedCursorInfo info, Stat stat) {
updateCursorLedgerStat(info, stat);
}
@Override
public void operationFailed(MetaStoreException e) {
if (log.isDebugEnabled()) {
log.debug(
"[{}] Failed to refresh cursor metadata-version for {} due "
+ "to {}", ledger.name, name, e.getMessage());
}
}
});
}
}
callback.operationFailed(e);

Modifications

  • managedLedger.close triggers the cursor.close, so remove code cursor.close
  • if managedLedger.close fail, just retry, it will be ok

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 7, 2022
@poorbarcode poorbarcode force-pushed the flaky_asyncMarkDeleteAndClose_2 branch from 4db61d1 to 76abd0e Compare November 10, 2022 13:36
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022

Codecov Report

Merging #18362 (fd24928) into master (8b404d4) will decrease coverage by 0.02%.
The diff coverage is 6.12%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18362      +/-   ##
============================================
- Coverage     47.15%   47.12%   -0.03%     
+ Complexity    10432    10411      -21     
============================================
  Files           697      698       +1     
  Lines         68023    67986      -37     
  Branches       7284     7271      -13     
============================================
- Hits          32074    32041      -33     
- Misses        32367    32400      +33     
+ Partials       3582     3545      -37     
Flag Coverage Δ
unittests 47.12% <6.12%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.98% <0.00%> (+0.71%) ⬆️
...n/java/org/apache/pulsar/client/util/NoOpLock.java 0.00% <0.00%> (ø)
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.12% <7.69%> (+0.28%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerBase.java 21.93% <16.66%> (-0.02%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 10.65% <0.00%> (-44.27%) ⬇️
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 46.03% <0.00%> (-14.86%) ⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-12.35%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 70.76% <0.00%> (-7.70%) ⬇️
.../pulsar/broker/service/PublishRateLimiterImpl.java 58.00% <0.00%> (-6.00%) ⬇️
... and 53 more

@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@tisonkun tisonkun changed the title [fix][test]fix flaky test ManagedLedgerBkTest.asyncMarkDeleteAndClose [fix][test] flaky test ManagedLedgerBkTest.asyncMarkDeleteAndClose Nov 15, 2022
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Although I suspect we may not retry on failure, the change is OK to merge.

@Technoboy- Technoboy- closed this Nov 19, 2022
@Technoboy- Technoboy- reopened this Nov 19, 2022
@Technoboy- Technoboy- force-pushed the flaky_asyncMarkDeleteAndClose_2 branch from 76abd0e to fd24928 Compare November 19, 2022 04:12
@Technoboy- Technoboy- merged commit 46ffb39 into apache:master Nov 19, 2022
@poorbarcode poorbarcode deleted the flaky_asyncMarkDeleteAndClose_2 branch November 21, 2022 05:06
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: ManagedLedgerBkTest.asyncMarkDeleteAndClose
5 participants