[fix][broker] Fix delete namespace fail by a In-flight topic

[mattisonchao]

Fixes #19083
Fixes #11866

Motivation

Currently, the force 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

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.
• Add retry to resolve some topics that were not created successfully(in metadata) during the deletion.
• Add a test case to test if the topic object is left behind.

Verifying this change

• Make sure that the change passes the CI checks.

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

[eolivelli]

This patch is related
#19097

@dlg99 @aymkhalil PTAL

[eolivelli]

I am not sure that this holds "Always assume the existence of an event topic "

[dlg99]

@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 #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.

[mattisonchao]

@dlg99

swap order of steps 2 and 3 (mark deleted first, do the rest, unmark on error).
In combo with #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.

[dlg99]

@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.

[mattisonchao]

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.

Yes, that is what I'm changing right now. I would move the markDelete before listing topics and handling the duplicate deletion exception.

[mattisonchao]

@eolivelli @dlg99 Please review it again. I've changed the logic to retry to help delete in-flight topics.

[eolivelli]

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

[eolivelli]

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.

[mattisonchao]

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.

[eolivelli]

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

[dlg99]

LGTM assuming you address Enrico's comments + CI passes (currently "You have 1 Checkstyle violation")

[eolivelli]

The retry logic is broker

please check my comments

[eolivelli]

LGTM

[eolivelli]

LGTM