[Issue 6043] Support force deleting subscription #6383
Conversation
There was a problem hiding this comment.
@murong00 Currently we don't have a flag to control to enable or disable the subscription auto-creation, and subscription auto-creation is enabled by default. So, if the subscription that has some active consumers, then delete the subscription forcefully, the subscription will create again when the consumer reconnects to the broker. Looks we'd better expose the ability for users to disable the subscription auto-creation.
| admin.topics().deleteSubscription(persistentTopicName, subName, true); | ||
|
|
||
| // delete the subscription successfully | ||
| assertEquals(admin.topics().getSubscriptions(persistentTopicName).size(), 0); |
There was a problem hiding this comment.
This will be a flaky test since the consumer can create a subscription when they reconnect to the broker.
There was a problem hiding this comment.
yes, maybe we can disable this line in this pr first and then refine it later when the ability to disable the subscription auto-creation is available?
There was a problem hiding this comment.
I hava fixed the unit test to cover this.
|
@codelipenghui yes, I hava already opened an issue (#6394) to track this, I am planning to fix it by a seperated pull request. |
murong00
force-pushed
the
branch-6383
branch
from
09e62b4
to
f326504
Compare
|
@codelipenghui please help to review again, thanks. |
|
@codelipenghui @sijie @jiazhai Since #6394 is already fixed, I hava refined the unit test. Please help to review it again, thanks. |
| } | ||
|
|
||
| private void internalDeleteSubscriptionForNonPartitionedTopicForcefully(AsyncResponse asyncResponse, String subName, boolean authoritative) { | ||
| validateAdminAccessForSubscriber(subName, authoritative); |
There was a problem hiding this comment.
Should handle exception here since we need to resume the asyncResponse
| if (topicName.isPartitioned()) { | ||
| internalDeleteSubscriptionForNonPartitionedTopicForcefully(asyncResponse, subName, authoritative); | ||
| } else { | ||
| PartitionedTopicMetadata partitionMetadata = getPartitionedTopicMetadata(topicName, authoritative, false); |
There was a problem hiding this comment.
Should handle exception here since we need to resume the asyncResponse
|
@codelipenghui ok, I will refine this pr after #6483 is merged. |
|
@codelipenghui I hava addressed your comments, please help to take a look. |
| * | ||
| * @return a future that can be used to track when the subscription is deleted | ||
| */ | ||
| CompletableFuture<Void> deleteSubscriptionAsync(String topic, String subName); | ||
| CompletableFuture<Void> deleteSubscriptionAsync(String topic, String subName, boolean force); |
There was a problem hiding this comment.
This is an API breaking change. You can't directly change the method signature. Please add an override method.
|
@sijie Please help to review this again, thanks. |
### Motivation Fixes apache#6043 ### Modifications Add method `deleteForcefully` to support force deleting subscription.
Motivation
Fixes #6043
Modifications
Add method
deleteForcefullyto support force deleting subscription.