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] [broker] Fix race-condition causing repeated delete topic #23522

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Oct 28, 2024

Fixes #23474

Motivation

Background

A lock named isClosingOrDeleting prevents concurrently deleting/closing topic


Issue 1: repeated delete a topic caused by a race condition

The code that deleting the topic is like below

CompletableFuture deleteFuture = () -> {
    CompletableFuture res = new CompletableFuture();
    ....
    if (success) {
        res.complete();
    } else {
        res.failed();
        // Highlight: release the lock `isClosingOrDeleting`.
        //Release the lock the first time.
        unfenceTopicToResume();
    }
};

deleteFuture.whenComplete((__ ,ex) -> {
    if (ex != null) {
        // Highlight: release the lock `isClosingOrDeleting`.
        //Release the lock the second time.
        unfenceTopicToResume();
    }
});

The code in PersistentTopic.delete acquires the lock once, but releases it twice, which breaks the locker, and leads to multiple deleting runs at the same time.


Issue 2: MetaStoreImpl.getException wrap a CompletionException to MetaStoreException deactivates the checker of deleting ML in the method PersistentTopic.delete

There is a checker that broker will assumed the ML deleting is succeed if it has been deleted before, such as bellow:

if (exception.getCause()  instanceof MetadataStoreException.NotFoundException) {
    future.complete(ctx);
} 

But MetaStoreImpl.getException wrap a CompletionException to MetaStoreException, causing the catch block to get an error like this MetaStoreException -> CompletionException -> NotFoundException, it breaks the checker above.

Modifications

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode changed the title Fix/repeat delete topic [fix] [broker] Fix race-condition causing repeated delete topic Oct 28, 2024
@poorbarcode poorbarcode self-assigned this Oct 28, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 28, 2024
@poorbarcode poorbarcode requested a review from lhotari October 28, 2024 08:59
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari requested a review from nodece October 28, 2024 09:24
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.34%. Comparing base (bbc6224) to head (36cc57f).
Report is 701 commits behind head on master.

Files with missing lines Patch % Lines
...sar/broker/service/persistent/PersistentTopic.java 50.00% 4 Missing ⚠️
.../apache/bookkeeper/mledger/impl/MetaStoreImpl.java 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23522      +/-   ##
============================================
+ Coverage     73.57%   74.34%   +0.77%     
- Complexity    32624    34938    +2314     
============================================
  Files          1877     1943      +66     
  Lines        139502   147011    +7509     
  Branches      15299    16199     +900     
============================================
+ Hits         102638   109299    +6661     
- Misses        28908    29284     +376     
- Partials       7956     8428     +472     
Flag Coverage Δ
inttests 27.64% <7.14%> (+3.05%) ⬆️
systests 24.42% <7.14%> (+0.10%) ⬆️
unittests 73.71% <57.14%> (+0.86%) ⬆️

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

Files with missing lines Coverage Δ
.../apache/bookkeeper/mledger/impl/MetaStoreImpl.java 85.36% <66.66%> (-0.55%) ⬇️
...sar/broker/service/persistent/PersistentTopic.java 79.93% <50.00%> (+1.47%) ⬆️

... and 652 files with indirect coverage changes

@Technoboy- Technoboy- added this to the 4.1.0 milestone Oct 29, 2024
@poorbarcode poorbarcode merged commit 7b80f01 into apache:master Oct 29, 2024
52 checks passed
poorbarcode added a commit that referenced this pull request Oct 29, 2024
poorbarcode added a commit that referenced this pull request Oct 29, 2024
poorbarcode added a commit that referenced this pull request Oct 29, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 5, 2024
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Nov 6, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 7, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 7, 2024
lhotari pushed a commit that referenced this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: BadVersionExceptions in OneWayReplicatorUsingGlobalZKTest.cleanup
6 participants