-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 delete_when_subscriptions_caught_up doesn't work while have active consumers #18283
[fix][broker] fix delete_when_subscriptions_caught_up doesn't work while have active consumers #18283
Conversation
…ile have active consumers
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18283 +/- ##
============================================
+ Coverage 38.97% 40.13% +1.15%
- Complexity 8311 8651 +340
============================================
Files 683 687 +4
Lines 67325 67437 +112
Branches 7217 7225 +8
============================================
+ Hits 26239 27063 +824
+ Misses 38079 37349 -730
- Partials 3007 3025 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
|
// 2. We want to kick out everyone and forcefully delete the topic. | ||
// In this case, we shouldn't care if the usageCount is 0 or not, just proceed | ||
if (currentUsageCount() == 0 || (closeIfClientsConnected && !failIfHasSubscriptions)) { | ||
if (currentUsageCount() == 0) { |
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.
Seems we need to keep the original condition (closeIfClientsConnected && !failIfHasSubscriptions))
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.
I feel the current code is correct. Because we have called disconnection operation before. If there are still any connections at the current time, it means that a new connection has come in, we should consider that there is a concurrency problem and give up deleting the topic.
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.
Yes, you are right @poorbarcode
The closeIfClientsConnected
and failIfHasSubscriptions
is validated before we reach here. It looks like duplicated validation.
// 2. We want to kick out everyone and forcefully delete the topic. | ||
// In this case, we shouldn't care if the usageCount is 0 or not, just proceed | ||
if (currentUsageCount() == 0 || (closeIfClientsConnected && !failIfHasSubscriptions)) { | ||
if (currentUsageCount() == 0) { |
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.
I feel the current code is correct. Because we have called disconnection operation before. If there are still any connections at the current time, it means that a new connection has come in, we should consider that there is a concurrency problem and give up deleting the topic.
} else if (!closeIfClientsConnected && currentUsageCount() != 0 && !failIfHasBacklogs) { | ||
return FutureUtil.failedFuture(new TopicBusyException( | ||
"Topic has " + currentUsageCount() + " connected producers/consumers")); |
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.
It needs to fail as long as there are connections, so it has nothing to do with !failIfHasBacklogs
.
Or if failIfHasBacklogs is triggered, the error message returned should not be like this
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.
Hi @315157973
When failIfHasBacklogs is true, the expected behavior is that:
- failure of any producer exists
- if any consumer exists, close connections
Actually, the code should be like this:
if ( !closeIfClientsConnected && currentUsageCount() ){
fail...
} else if ( !closeIfClientsConnected !failIfHasBacklogs && anyProducerExists() ){
fail...
}
This code may not be easy to understand from the context, but the logic is correct
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.
There is also a concurrency problem that may lead to the incorrect deletion of existing producer topics. E.g:
checkGC |
new producer registry |
---|---|
ensure no producer exists | |
delete topic ( false , true , false ) |
|
ensure no backlog | |
new producer registry | |
fence topic | |
disconnect all clients | |
delete topic |
The above flow shows the case: producer exists but the topic is deleted. Would it be better to add a double-check?
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.
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.
We have write lock here https://github.com/apache/pulsar/pull/18283/files#diff-5edf14cc6f25857d0cfdd26b2d3b3141230ecfb0dfa95aebf7583fd76ede4c4bR1161
Ah, yes you are right
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
could you please cherry-pick this PR to branch-2.9? thanks. |
#18320 has cherry-picked this one to branch-2.11 |
f5c9354 cherry-picked to branch-2.10 |
3cf167a cherry-picked to branch-2.9 |
Motivation
The current behavior for the
delete_when_subscriptions_caught_up
strategy is not expected. The active consumer will not be closed even if users enabledelete_when_subscriptions_caught_up
and there are no backlogs for the topic.It should be the part that #6077 has missed. And Sijie has mentioned in the comment #6077 (review).
To correct the behavior of
delete_when_subscriptions_caught_up
Modifications
Close active consumers if
delete_when_subscriptions_caught_up
is applied and there are no backlogs for the topic. So that the topic can be cleaned up properly by the topic GC thread.Verifying this change
Updated the existing test.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: codelipenghui#17