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

[Branch-2.8] Fix Broker HealthCheck Endpoint Exposes Race Conditions. #14618

Merged
merged 4 commits into from
Mar 10, 2022

Conversation

Technoboy-
Copy link
Contributor

Cherry-pick #14367

Master Issue: #14362

Motivation

See #14362

According to relative PR #7724, we will force delete all subscriptions when calling healthCheck REST API. but it has a race condition when two threads call this API.
Please consider this case:

Thread A: Clean up all subscriptions, then create a reader.
Thread B: Prepare to force delete all subscriptions.

So, in this case, the reader of thread A is deleted and then an NPE or other exception occurs.

Modifications

  • Use Completable#handle to fix problem 1, the reader needs to be closed regardless of whether there is an exception.
  • Recheck the subscription after closing reading, and force deletion if it still exists after closing reading.
  • Added multi-threaded tests for health checks.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • no-need-doc

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli
Copy link
Contributor

@Technoboy-
I little suggestion for the future:
when you cherry pick a patch from someone else it is a good practice to start by cherry picking the original patch and try to keep the original author, otherwise it seems that you are the original author, in this case it is @mattisonchao

I am looking here
https://github.com/apache/pulsar/pull/14618/commits

this is the original commit
4f1e39b

it is fine to amend it and adapt it to the target branch, but my point is that in the git history @mattisonchao should be listed as author of the patch (whenever it is possible).

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

committing as soon as CI is green

@Technoboy-
Copy link
Contributor Author

@Technoboy- I little suggestion for the future: when you cherry pick a patch from someone else it is a good practice to start by cherry picking the original patch and try to keep the original author, otherwise it seems that you are the original author, in this case it is @mattisonchao

I am looking here https://github.com/apache/pulsar/pull/14618/commits

this is the original commit 4f1e39b

it is fine to amend it and adapt it to the target branch, but my point is that in the git history @mattisonchao should be listed as author of the patch (whenever it is possible).

Yes, I see. Thanks. But there are a lot of conflicts when cherry-picking. So I have to rewrite it according to the original one. We can add the author info when merging. (I'm not for any commits record here, but for speeding up the release.)

@Technoboy-
Copy link
Contributor Author

LGTM

committing as soon as CI is green

Ok, fixed.

@codelipenghui codelipenghui merged commit 32f8065 into apache:branch-2.8 Mar 10, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 10, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 10, 2022
@Technoboy- Technoboy- deleted the cherry-pick-14367 branch August 10, 2022 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants