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/08/26 22:51:10 UTC
[GitHub] [pulsar] dlg99 opened a new pull request, #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
dlg99 opened a new pull request, #17308:
URL: https://github.com/apache/pulsar/pull/17308
### Motivation
https://github.com/datastax/pulsar/issues/112
Forced delete of partitioned topic with active consumer on the namespace where the topic autocreate is enabled leaves the namespace in the state where one cannot create partitioned topic with the same name (because it exists already) and cannot delete it (because it does not exist at the same time)
### Modifications
Don't allow deletion in this case until all consumers/producers disconnected.
I experimented with option to not allow autocreate after deletion of partitioned topics but that ends up in the reasons that led to https://github.com/apache/pulsar/pull/14920 + tricky corner cases between metadata updates.
### Verifying this change
This change added tests
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
nothing AFAICT
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
Bug fix
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `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 merged pull request #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #17308:
URL: https://github.com/apache/pulsar/pull/17308
--
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] poorbarcode commented on pull request #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1231911843
This PR can not solve every scenario:
The cmd delete topic is executed between these two instructions: `consumer lookup` and `consumer subscribe`, even if the partitioned topic is deleted successfully, but the client already has the topic-meta(which has been deleted), then the consumer subscribes with the topic name "topic-partition-x". You can reproduce like this:
```
1. create partitioned topic "tp_test"
2. consumer lookup
3. delete topic
4. consumer subscribe
5."tp_test-partition-x" created
```
--
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 #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1232903489
@poorbarcode this is a good catch indeed!
Thinking more about this case the "problem" is related to the sequences of things that happen in the two operations:
- delete a partitioned topic
- automatic creation of a partitioned topic
We must prevent these two things to be performed concurrently
Ideally the two operations should be performed in reverse order:
when you delete the topic the last thing that you delete is the first thing that "create partitioned topic" checks
so until the deletion is finished a new creation cannot fails
--
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] poorbarcode commented on a diff in pull request #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#discussion_r958734425
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1139,6 +1139,13 @@ private CompletableFuture<Void> delete(boolean failIfHasSubscriptions,
.map(PersistentSubscription::getName).toList();
return FutureUtil.failedFuture(
new TopicBusyException("Topic has subscriptions did not catch up: " + backlogSubs));
+ } else if (TopicName.get(topic).isPartitioned()
+ && (getProducers().size() > 0 || getNumberOfConsumers() > 0)
+ && getBrokerService().isAllowAutoTopicCreation(topic)) {
Review Comment:
This PR can not solve every scenario:
The cmd delete topic is executed between these two instructions: `consumer lookup` and `consumer subscribe`, even if the partitioned topic is deleted successfully, but the client already has the topic-meta(which has been deleted), then the consumer subscribes with the topic name "topic-partition-x". You can reproduce like this:
```
1. create partitioned topic "tp_test"
2. consumer lookup
3. delete topic
4. consumer subscribe
5."tp_test-partition-x" created
```
--
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] poorbarcode commented on pull request #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1241530818
Hi @eolivelli @dlg99
I think we should revert this PR.
System topics are always created automatically, and `isAllowAutoTopicCreation(system topic)` always returns true. If a namespace contains any partitioned system topic, this namespace can not be deleted( After this PR is Merged ).
--
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 #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#discussion_r956541403
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ExclusiveProducerTest.java:
##########
@@ -331,7 +332,12 @@ public void topicDeleted(String ignored, boolean partitioned) throws Exception {
p1.send("msg-1");
if (partitioned) {
- admin.topics().deletePartitionedTopic(topic, true);
+ try {
+ admin.topics().deletePartitionedTopic(topic, true);
+ fail("expected error because partitioned topic has active producer");
+ } catch (PulsarAdminException.ServerSideErrorException e) {
Review Comment:
ServerSideError is not very user friendly.
I would expect 'Conflict' or the same thing that happens when you do not use the 'force' option
--
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 #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1233114990
I also think this change mitigates the problems we've encountered in prod; before implementing this I experimented with different approach (as mentioned in the description of the 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] eolivelli commented on pull request #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1229521264
(As discussed offline) As a follow up work we should disable automatic topic creation before deleting a namespace, this way we will allow users to delete namespaces and tenants more easily
--
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 #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1233035054
btw I think that this patch mitigates most of the usecases that happen in real world (and we saw it a lot of them in some production clusters after upgrading to 2.10)
--
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 #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#discussion_r956605268
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ExclusiveProducerTest.java:
##########
@@ -331,7 +332,12 @@ public void topicDeleted(String ignored, boolean partitioned) throws Exception {
p1.send("msg-1");
if (partitioned) {
- admin.topics().deletePartitionedTopic(topic, true);
+ try {
+ admin.topics().deletePartitionedTopic(topic, true);
+ fail("expected error because partitioned topic has active producer");
+ } catch (PulsarAdminException.ServerSideErrorException e) {
Review Comment:
@eolivelli I return the same TopicBusyException as other cases when topic deletion is not possible.
If it needs to be translated into some other error for pulsar admin/rest API I suggest we create a bug and resolve this in a different PR
https://github.com/apache/pulsar/blob/34c2262d16591fe9af6ce88ad58c72e3c017d4f1/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L1129-L1148
--
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 #17308: [fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17308:
URL: https://github.com/apache/pulsar/pull/17308#issuecomment-1241574626
@poorbarcode you are right, I think that we must revert this patch.
Let me send a 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