You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/03/09 08:21:16 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14618: [Branch-2.8] Fix Broker HealthCheck Endpoint Exposes Race Conditions.
Technoboy- opened a new pull request #14618:
URL: https://github.com/apache/pulsar/pull/14618
Cherry-pick https://github.com/apache/pulsar/pull/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
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `no-need-doc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #14618: [Branch-2.8] Fix Broker HealthCheck Endpoint Exposes Race Conditions.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14618:
URL: https://github.com/apache/pulsar/pull/14618
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #14618: [Branch-2.8] Fix Broker HealthCheck Endpoint Exposes Race Conditions.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #14618:
URL: https://github.com/apache/pulsar/pull/14618#issuecomment-1062934798
@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
https://github.com/apache/pulsar/commit/4f1e39b6921ea401b8c27f17a041d06d85f8abf8
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14618: [Branch-2.8] Fix Broker HealthCheck Endpoint Exposes Race Conditions.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14618:
URL: https://github.com/apache/pulsar/pull/14618#issuecomment-1063580105
> @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](https://github.com/apache/pulsar/commit/4f1e39b6921ea401b8c27f17a041d06d85f8abf8)
>
> 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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14618: [Branch-2.8] Fix Broker HealthCheck Endpoint Exposes Race Conditions.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14618:
URL: https://github.com/apache/pulsar/pull/14618#issuecomment-1063582662
> LGTM
>
> committing as soon as CI is green
Ok, fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org