You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "mattisonchao (via GitHub)" <gi...@apache.org> on 2023/01/31 14:03:29 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #19374: [fix][broker] Fix delete namespace fail by a race condition
mattisonchao opened a new pull request, #19374:
URL: https://github.com/apache/pulsar/pull/19374
Fixes #19083
Fixes #11866
### Motivation
Currently, the delete namespace operation has the following steps:
1. Do pre-check
2. Get full of topics
3. Mark the namespace as `deleted` to avoid creating the topic again by client lookup or reconnect.
4. Delete all of the user-created topics
5. Delete all system topics.
6. Delete namespace event topics.
7. clean up namespace metadata and resources
A race condition will make step 2 unable to get full of topics. e.g:
| - | Thread A | Thread B |
|--------|------------------------------------------------------------------------------------------|-------------------------------------------------------------|
| time 1 | Got full of topics | Trying to create a new topic and passed the `deleted` check |
| time 2 | Mark `deleted` | Do other checks |
| time 3 | Do the rest of steps 4,5,6 | Created managed ledger and has persistent info to metadata |
| time 4 | Step 7, Got exception `Directory not empty for /managed-ledgers/test-tenant/test-ns2/persistent` | Do the rest work |
| time 5 | Return the exception | Topic created |
This problem also exists for user-created topics, but I don't think we need to deal with them. The user can run the command multiple times and it will clean up successfully. Otherwise, we need to use a heavily distributed lock to ensure that.
However, we need to do something about the internal `change_event` topic. we can always assume the existence of a `change_event` topic to avoid race conditions where the topic passes the check but is not yet persisted in the metadata.
I've tested in my local env 1k times. It works great for me.
<img width="657" alt="image" src="https://user-images.githubusercontent.com/74767115/215781535-c993e7f3-aa8d-406e-b720-0aa97f72dfab.png">
### Modifications
> Deleting topic xxxx because local cluster is not part of global namespace repl list
- Remove `namespacePolicies.deleted` logic to ensure the new topic will not be deleted by `PersistenTopic#checkReplication`.
- Always assume the existence of an event topic to avoid race conditions where the topic passes the check but is not yet persisted in the metadata.
- Add a test case to test if the subject name object is left behind.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
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 #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1410729346
I am not sure that this holds "Always assume the existence of an event topic "
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1091979931
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java:
##########
@@ -1653,6 +1654,9 @@ public void testDeleteNamespaceWithTopicPolicies() throws Exception {
admin.topics().createNonPartitionedTopic(namespace + "/tobedeleted");
// verify namespace can be deleted even without topic policy events
admin.namespaces().deleteNamespace(namespace, true);
+ final CompletableFuture<Optional<Topic>> eventTopicFuture =
Review Comment:
Add a test case to test if the topic object is left behind.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1091977914
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
Remove `namespacePolicies.deleted` logic to ensure the new topic will not be deleted by `PersistenTopic#checkReplication`.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1093779898
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -346,7 +346,7 @@ public CompletableFuture<Void> initialize() {
Policies policies = optPolicies.get();
- this.updateTopicPolicyByNamespacePolicy(policies);
+ updateTopicPolicyByNamespacePolicy(policies);
Review Comment:
```suggestion
this.updateTopicPolicyByNamespacePolicy(policies);
```
--
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 #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1410439224
This patch is related
https://github.com/apache/pulsar/pull/19097
@dlg99 @aymkhalil PTAL
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092525233
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
Hi @eolivelli
I don't understand what you mean.
Will removing this `deleted` verification affect anything?
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092525233
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
Hi @eolivelli
I don't understand what you mean.
Will removing this test affect anything?
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092526464
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/SystemTopicNames.java:
##########
@@ -81,7 +81,7 @@ public static boolean isTopicPoliciesSystemTopic(String topic) {
if (topic == null) {
return false;
}
- return TopicName.get(topic).getLocalName().equals(NAMESPACE_EVENTS_LOCAL_NAME);
+ return TopicName.get(topic).getLocalName().startsWith(NAMESPACE_EVENTS_LOCAL_NAME);
Review Comment:
Ok, I can revert this change.
--
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 a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1106771832
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +208,48 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ RetryUtil.retryAsynchronously(() -> internalDeleteNamespaceAsync0(force),
+ new BackoffBuilder()
+ .setInitialTime(200, TimeUnit.MILLISECONDS)
+ .setMandatoryStop(15, TimeUnit.SECONDS)
Review Comment:
distributed locks are very expensive and in any case you will have to deal with timeouts.
We should make the operations idempotent or chain them.
if a new operation comes and the deletion is already in progress we must wait for the result of the pending operation
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1107341804
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +206,46 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
- CompletableFuture<Policies> preconditionCheck = precheckWhenDeleteNamespace(namespaceName, force);
- return preconditionCheck
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ internalRetryableDeleteNamespaceAsync0(force, 5, future);
+ return future;
+ }
+ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTimes,
+ @Nonnull CompletableFuture<Void> callback) {
+ if (retryTimes == 0) {
+ // drop out recursive
Review Comment:
Yes, I think we can move this check before doing recursive.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -299,7 +314,23 @@ protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
return CompletableFuture.completedFuture(null);
})
).collect(Collectors.toList())))
- .thenCompose(ignore -> internalClearZkSources());
+ .thenCompose(ignore -> internalClearZkSources())
+ .whenComplete((result, error) -> {
+ if (error != null) {
+ final Throwable rc = FutureUtil.unwrapCompletionException(error);
+ if (rc instanceof MetadataStoreException) {
+ if (rc.getCause() != null && rc.getCause() instanceof KeeperException.NotEmptyException) {
Review Comment:
Sure, Thanks to you. 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
[GitHub] [pulsar] dlg99 commented on pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1410904484
@mattisonchao can this be done slightly differently:
swap order of steps 2 and 3 (mark deleted first, do the rest, unmark on error).
In combo with https://github.com/apache/pulsar/pull/19097 this should work good enough.
There is still a chance that a topic creation passed the check before ns deletion started, and completed after the ns is marked deleted. I don't think we can solve it easily, this will require either distributed locking or serialization of ns/topic metadata operations e.g. via global system topic.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092538239
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
@eolivelli
If I don't remove this check, the creating topic in the deleted namespace will automatically be forcefully deleted by `checkReplication` method because the `replicationClusters` is null. See the logic details:
https://github.com/apache/pulsar/blob/17c58a539315cc4ea39655d4328c5caf55f87d3d/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L1468-L1479
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1107216419
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -317,7 +316,21 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
return CompletableFuture.completedFuture(null);
})
).collect(Collectors.toList())))
- .thenCompose(ignore -> internalClearZkSources());
+ .thenCompose(ignore -> internalClearZkSources())
+ .whenComplete((result, error) -> {
+ if (error != null) {
+ final Throwable rc = FutureUtil.unwrapCompletionException(error);
+ if (rc instanceof MetadataStoreException) {
+ if (rc.getCause() != null && rc.getCause() instanceof KeeperException.NotEmptyException) {
+ internalRetryableDeleteNamespaceAsync0(force, retryTimes, callback);
Review Comment:
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
[GitHub] [pulsar] mattisonchao closed pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao closed pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
URL: https://github.com/apache/pulsar/pull/19374
--
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 a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092224072
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/SystemTopicNames.java:
##########
@@ -81,7 +81,7 @@ public static boolean isTopicPoliciesSystemTopic(String topic) {
if (topic == null) {
return false;
}
- return TopicName.get(topic).getLocalName().equals(NAMESPACE_EVENTS_LOCAL_NAME);
+ return TopicName.get(topic).getLocalName().startsWith(NAMESPACE_EVENTS_LOCAL_NAME);
Review Comment:
This is strange.
it means that sometimes we call this function on the logical name of the partitioned topic and sometimes on a partition.
are we changing the behavior ?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
with https://github.com/apache/pulsar/pull/19097 we will be sure that "namespacePolicies.deleted" is authoritative.
if we are deleting the namespace then we should do nothing
cc @dlg99 @aymkhalil
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1093780209
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -346,7 +346,7 @@ public CompletableFuture<Void> initialize() {
Policies policies = optPolicies.get();
- this.updateTopicPolicyByNamespacePolicy(policies);
Review Comment:
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
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092538239
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
@eolivelli
If I don't remove this check, the creating topic in the deleted namespace will automatically be forcefully deleted by `checkReplication` method and conflict with namespace deletion. because the `replicationClusters` is null. See the logic details:
https://github.com/apache/pulsar/blob/17c58a539315cc4ea39655d4328c5caf55f87d3d/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L1468-L1479
Obviously, This behaviour doesn't have any problem. We just delete the topic at the same time twice. but I don't think it's a good approach to delete this kind of topic(`the creating topic in the deleted namespace will automatically be forcefully deleted `) with this method, because it is randomness and that can only confuse some internal behaviour.
Maybe I can revert this change and then handle the conflict exception. what do you think?
/cc @eolivelli @dlg99
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092525233
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
Hi @eolivelli
I don't understand what you mean.
Will removing this `deleted` verification affect anything?
--
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- merged pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #19374:
URL: https://github.com/apache/pulsar/pull/19374
--
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 a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1107294622
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +206,46 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
- CompletableFuture<Policies> preconditionCheck = precheckWhenDeleteNamespace(namespaceName, force);
- return preconditionCheck
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ internalRetryableDeleteNamespaceAsync0(force, 5, future);
+ return future;
+ }
+ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTimes,
+ @Nonnull CompletableFuture<Void> callback) {
+ if (retryTimes == 0) {
+ // drop out recursive
Review Comment:
we should fail here and we should not never reach here
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -299,7 +314,23 @@ protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
return CompletableFuture.completedFuture(null);
})
).collect(Collectors.toList())))
- .thenCompose(ignore -> internalClearZkSources());
+ .thenCompose(ignore -> internalClearZkSources())
+ .whenComplete((result, error) -> {
+ if (error != null) {
+ final Throwable rc = FutureUtil.unwrapCompletionException(error);
+ if (rc instanceof MetadataStoreException) {
+ if (rc.getCause() != null && rc.getCause() instanceof KeeperException.NotEmptyException) {
Review Comment:
here we must check that the number of retries is exhausted and do not perform the recursion
--
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 a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1093077940
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +208,48 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ RetryUtil.retryAsynchronously(() -> internalDeleteNamespaceAsync0(force),
+ new BackoffBuilder()
+ .setInitialTime(200, TimeUnit.MILLISECONDS)
+ .setMandatoryStop(15, TimeUnit.SECONDS)
Review Comment:
I don't think it is good to set a time based limit, also because a namespace may have thousand of topics and it may really take more that 15 seconds.
we should set a value in the order of minutes (10 minutes ?)
and we should log something at every trial, this way from the logs it will be clear that something is going on
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +208,48 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ RetryUtil.retryAsynchronously(() -> internalDeleteNamespaceAsync0(force),
+ new BackoffBuilder()
+ .setInitialTime(200, TimeUnit.MILLISECONDS)
+ .setMandatoryStop(15, TimeUnit.SECONDS)
Review Comment:
what happens if the user gives up waiting and then it issues again the same command while the backoff is still running ? Ideally everything should work well and the second execution should wait for the previous execution to complete. The expectation for the user is that when the command completes the namespace is deleted.
--
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 a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1106810866
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -317,7 +316,21 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
return CompletableFuture.completedFuture(null);
})
).collect(Collectors.toList())))
- .thenCompose(ignore -> internalClearZkSources());
+ .thenCompose(ignore -> internalClearZkSources())
+ .whenComplete((result, error) -> {
+ if (error != null) {
+ final Throwable rc = FutureUtil.unwrapCompletionException(error);
+ if (rc instanceof MetadataStoreException) {
+ if (rc.getCause() != null && rc.getCause() instanceof KeeperException.NotEmptyException) {
+ internalRetryableDeleteNamespaceAsync0(force, retryTimes, callback);
Review Comment:
please log here that you are going to retry
--
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] mattisonchao commented on pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1411119516
@dlg99
>swap order of steps 2 and 3 (mark deleted first, do the rest, unmark on error).
In combo with https://github.com/apache/pulsar/pull/19097 this should work good enough.
> There is still a chance that a topic creation passed the check before ns deletion started, and completed after the ns is marked deleted. I don't think we can solve it easily, this will require either distributed locking or serialization of ns/topic metadata operations e.g. via global system topic.
As you said. the topic still has the chance to go into the race condition.
> I don't think we can solve it easily,
We don't need to solve the user-created topic but we should make sure to clean up the system topic.
--
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] dlg99 commented on pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1411264444
@mattisonchao what's happening in this PR as I understand:
1. delete all topics + all system topics except for the events one.
2. Then delete the events topic
3. but that brings in some problems with handling of ns.deleted metadata (because of rpc?) so let's remove that logic in some places.
This does not sound like a comprehensive solution (can't topic creation happen after p.2 and still cause namespace deletion to fail?) plus it breaks contract for handling of ns.deleted as @eolivelli moted.
On another note, events topic is being deleted using `admin.topics().deletePartitionedTopicAsync` while for other partitioned topics we use `namespaceResources().getPartitionedTopicResources().deletePartitionedTopicAsync` (in internalDeletePartitionedTopicsAsync). Is it done on purpose?
I'd leave ns.deleted handling logic as it is, set ns.deleted for namespace earlier before actually listing topics (seems to be the right thing to do logically).
Do the rest of the steps with retries/backoff - ns is marked as deleted so topic creations should settle at some point. List topics again and delete again. then delete the namespace metadata.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1091978748
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/SystemTopicNames.java:
##########
@@ -81,7 +81,7 @@ public static boolean isTopicPoliciesSystemTopic(String topic) {
if (topic == null) {
return false;
}
- return TopicName.get(topic).getLocalName().equals(NAMESPACE_EVENTS_LOCAL_NAME);
+ return TopicName.get(topic).getLocalName().startsWith(NAMESPACE_EVENTS_LOCAL_NAME);
Review Comment:
to match partitioned topic name `persistent://public/default/__change_event-partition-0`
--
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 a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1092859190
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -248,9 +245,8 @@ protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
hasNonSystemTopic = true;
allUserCreatedPartitionTopics.add(topic);
} else {
- if (SystemTopicNames.isTopicPoliciesSystemTopic(topic)) {
- partitionedTopicPolicy.add(topic);
- } else {
+ if (!TopicName.get(topic).getLocalName()
+ .startsWith(SystemTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)) {
Review Comment:
can we create an utility method in SystemTopicNames ?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -246,9 +246,6 @@ protected void updateTopicPolicyByNamespacePolicy(Policies namespacePolicies) {
if (log.isDebugEnabled()) {
log.debug("[{}]updateTopicPolicyByNamespacePolicy,data={}", topic, namespacePolicies);
}
- if (namespacePolicies.deleted) {
Review Comment:
I have checked the code again.
We can keep this change, it is harmless, the topic is deemed to be deleted, we can allow the policy to be updated
--
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] dlg99 commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1093670810
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -346,7 +346,7 @@ public CompletableFuture<Void> initialize() {
Policies policies = optPolicies.get();
- this.updateTopicPolicyByNamespacePolicy(policies);
Review Comment:
nit: unnecessary change for this PR
--
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] mattisonchao commented on pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1411752783
@eolivelli @dlg99 Please review it again. I change the logic to retry.
--
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] mattisonchao commented on pull request #19374: [fix][broker] Fix delete namespace fail by a race condition
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#issuecomment-1411296637
>I'd leave ns.deleted handling logic as it is, set ns.deleted for namespace earlier before actually listing topics (seems to be the right thing to do logically).
Yes, that is what I'm changing right now. I would move the `markDelete` before listing topics and handling the duplicate deletion exception.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1106766784
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +208,48 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ RetryUtil.retryAsynchronously(() -> internalDeleteNamespaceAsync0(force),
+ new BackoffBuilder()
+ .setInitialTime(200, TimeUnit.MILLISECONDS)
+ .setMandatoryStop(15, TimeUnit.SECONDS)
Review Comment:
Hi, @eolivelli
I am trying to use the number of retrying to instead back off.
>what happens if the user gives up waiting and then it issues again the same command while the backoff is still running ? Ideally everything should work well and the second execution should wait for the previous execution to complete. The expectation for the user is that when the command completes the namespace is deleted.
For this problem, we can give some kind of the same operation a distributed lock to avoid calling the same operation concurrently. I can send the discussion to the mailing list.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1107341804
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +206,46 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
- CompletableFuture<Policies> preconditionCheck = precheckWhenDeleteNamespace(namespaceName, force);
- return preconditionCheck
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ internalRetryableDeleteNamespaceAsync0(force, 5, future);
+ return future;
+ }
+ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTimes,
+ @Nonnull CompletableFuture<Void> callback) {
+ if (retryTimes == 0) {
+ // drop out recursive
Review Comment:
Yes, I think we can move this check to before doing recursive.
--
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] mattisonchao commented on a diff in pull request #19374: [fix][broker] Fix delete namespace fail by a In-flight topic
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19374:
URL: https://github.com/apache/pulsar/pull/19374#discussion_r1107341804
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +206,46 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
- CompletableFuture<Policies> preconditionCheck = precheckWhenDeleteNamespace(namespaceName, force);
- return preconditionCheck
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ internalRetryableDeleteNamespaceAsync0(force, 5, future);
+ return future;
+ }
+ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTimes,
+ @Nonnull CompletableFuture<Void> callback) {
+ if (retryTimes == 0) {
+ // drop out recursive
Review Comment:
Yes, I think we can move this check to front of doing recursive.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -204,23 +206,46 @@ protected CompletableFuture<List<String>> internalGetNonPersistentTopics(Policie
});
}
- @SuppressWarnings("unchecked")
- protected CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
- CompletableFuture<Policies> preconditionCheck = precheckWhenDeleteNamespace(namespaceName, force);
- return preconditionCheck
+ /**
+ * Delete the namespace and retry to resolve some topics that were not created successfully(in metadata)
+ * during the deletion.
+ */
+ protected @Nonnull CompletableFuture<Void> internalDeleteNamespaceAsync(boolean force) {
+ final CompletableFuture<Void> future = new CompletableFuture<>();
+ internalRetryableDeleteNamespaceAsync0(force, 5, future);
+ return future;
+ }
+ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTimes,
+ @Nonnull CompletableFuture<Void> callback) {
+ if (retryTimes == 0) {
+ // drop out recursive
Review Comment:
Yes, I think we can move this check to the front of doing recursive.
--
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