-
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
[Transaction] Fix delete sub then delete pending ack. #11023
[Transaction] Fix delete sub then delete pending ack. #11023
Conversation
@congbobo184 does this affect docs? |
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.
Thanks for working on this.
That triggers the creation of these additional topics? We should try the deletion of them only in such cases, otherwise we will impact every unsubscribe operations.
If there is no other way then I am fine with this approach
@@ -955,7 +956,31 @@ private void resetSubscriptionCursor(Subscription subscription, CompletableFutur | |||
@Override | |||
public CompletableFuture<Void> unsubscribe(String subscriptionName) { | |||
CompletableFuture<Void> unsubscribeFuture = new CompletableFuture<>(); | |||
getBrokerService().getManagedLedgerFactory().asyncDelete(TopicName.get(MLPendingAckStore |
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 should try this only in case we have transactions enabled?
Otherwise we are going to slow down every 'unsubscribe' operation with additional operations (look-up, metadata...)
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.
if we enabled after disable, we will not have method to delete this, so we should every time delete pendingAck
. I think we don't need to think about delete cursor speed.
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.
my main concern is about Readers
the reader will unsubscribe the temporary subscription.
but probably the Reader problem is different...should we create pending acks topics for Readers ? and for Non Durable subscriptions ?
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.
now, may don't think about the NonPersistentSubscription, this can be considered later.
don't need. |
…_pending_ack_store_log
…_pending_ack_store_log
…_pending_ack_store_log # Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
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.
@congbobo184 Do we need to handle the pending ack state when deleting the subscription
@congbobo184 Can you rebase to the master branch? |
/pulsarbot run-failure-checks |
@congbobo184 I am cutting the 2.8.1 release, could you rebase the master branch and resolve the conflicts? |
…ete_subscription_delete_pending_ack_store_log # Conflicts: # pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/pendingack/PendingAckPersistentTest.java
…_pending_ack_store_log
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
Fix delete sub then delete pending ack managedledger.
Fix delete sub then delete pending ack managedledger. (cherry picked from commit a50fe87)
Fix delete sub then delete pending ack managedledger.
Motivation
now delete topic sub don't delete pending ack topic. delete pending ack topic.
implement
delete pending ack topic.
Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)