[fix] Combination of autocreate + forced delete of partitioned topic with active consumer leaves topic metadata inconsistent.

[dlg99]

Motivation

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 #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)

• doc-not-needed

Bug fix

• doc
  (Your PR contains doc changes)

• doc-complete
  (Docs have been already added)

[eolivelli]

Good catch.

I left a comment about the error code. I think we should return something that is more user friendly as an Internal Error seems that the server is broken.

I have another thought, but I am not sure how we can fix it:
How can we help a user that doesn't have control on client applications but needs to delete a partitioned topic?
Maybe this is not a big deal because if you want to have control over the topics you turn off automatic creation at namespace level

[eolivelli]

ServerSideError is not very user friendly.
I would expect 'Conflict' or the same thing that happens when you do not use the 'force' option

[dlg99]

@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

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

Lines 1129 to 1148 in 34c2262

	if (isClosingOrDeleting) {
	log.warn("[{}] Topic is already being closed or deleted", topic);
	return FutureUtil.failedFuture(new TopicFencedException("Topic is already fenced"));
	} else if (failIfHasSubscriptions && !subscriptions.isEmpty()) {
	return FutureUtil.failedFuture(
	new TopicBusyException("Topic has subscriptions: " + subscriptions.keys()));
	} else if (failIfHasBacklogs && hasBacklogs()) {
	List<String> backlogSubs =
	subscriptions.values().stream()
	.filter(sub -> sub.getNumberOfEntriesInBacklog(false) > 0)
	.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)) {
	// to avoid inconsistent metadata as a result
	return FutureUtil.failedFuture(
	new TopicBusyException("Partitioned topic has active consumers or producers and "
	+ "auto-creation of topic is allowed"));

[eolivelli]

Lgtm

[eolivelli]

(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

[poorbarcode]

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

[eolivelli]

@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

[eolivelli]

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)

[dlg99]

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)

[poorbarcode]

Hi @eolivelli @dlg99

I think we should revert this PR.

System topics are always created automatically, and isAllowAutoTopicCreation(system topic) always returns true. Users will not stop the system topic manually before deleting a namespace. If a namespace contains any partitioned system topic, this namespace can not be deleted( After this PR is Merged ).

We should consider other ways to solve this problem

[eolivelli]

@poorbarcode you are right, I think that we must revert this patch.
Let me send a PR