-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Broker] Fix Broker HealthCheck Endpoint Exposes Race Conditions #14367
Conversation
I need more eyes to help me review this change to avoid regression. |
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
Show resolved
Hide resolved
Thanks for working on this @mattisonchao! |
@mattisonchao - I should have added in my review that your tests look good. |
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
Show resolved
Hide resolved
@michaeljmarshall @Technoboy- |
/pulsarbot rerun-failure-checks |
@codelipenghui @Technoboy- @eolivelli @lhotari @michaeljmarshall @Jason918 PTAL, when you have time ~ |
LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
great work !
I will review this within the next couple hours, thank you for your work @mattisonchao! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@codelipenghui if we cut a new rc for 2.10 is would be great to have this patch. |
@mattisonchao |
…che#14367) (cherry picked from commit 4f1e39b)
…che#14367) (cherry picked from commit 4f1e39b)
) (cherry picked from commit 4f1e39b)
…#14658) Original PR #14367 Because the PR #14367 is based on PR #14091, so I want to cherry-pick these two PRs to branch-2.9, the PR #13525 is also needed. --- Fix Issue: #14362 ### Motivation 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.
The #14658 cherry-pick this PR to |
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:
So, in this case, the reader of thread A is deleted and then an NPE or other exception occurs.
Modifications
Completable#handle
to fix problem 1, the reader needs to be closed regardless of whether there is an exception.Verifying this change
Documentation
no-need-doc