[fix][broker] revert remove duplicate topics name when deleteNamespace

[TakaHiR07]

Motivation

Related to #20683. When reading the code of master-branch and branch-2.10, I found we can not remove duplicate topics name when deleteNamespace, because the implementation of internalDeletePartitionedTopicsAsync() is different in two branch. In master-branch, if we remove topics name in allUserCreatedTopics and allSystemTopic, we actually not delete the topic. So the first time delete namespace will throw exception exactly.

branch-2.10 :

master-branch :

Modifications

revert the modification.

Verifying this change

• Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: TakaHiR07#14

[Technoboy-]

#20683 Only apply in the master branch.

[Technoboy-]

yes, right. When releasing 2.10, testDeleteNamespace is flaky test, we found it's related to delete duplicated topic. #20685
Then we cherry-pick the logic to the master by #20683. Unfortunately, internalDeletePartitionedTopicsAsync has changed in the master branch.

[codelipenghui]

Is it possible to have a test to protect the behavior?

[TakaHiR07]

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java

Lines 328 to 342 in e59c850

	if (rc instanceof MetadataStoreException) {
	if (rc.getCause() != null && rc.getCause() instanceof KeeperException.NotEmptyException) {
	log.info("[{}] There are in-flight topics created during the namespace deletion, "
	+ "retry to delete the namespace again.", namespaceName);
	final int next = retryTimes - 1;
	if (next > 0) {
	// async recursive
	internalRetryableDeleteNamespaceAsync0(force, next, callback);
	} else {
	callback.completeExceptionally(
	new RestException(Status.CONFLICT, "The broker still have in-flight topics"
	+ " created during namespace deletion, please try again."));
	// drop out recursive
	}
	return;

The retry logic make it hard to add test. Because this wrong behaviour also throw KeeperException.NotEmptyException, which would be caught and retry internalRetryableDeleteNamespaceAsync0(). Actually it should succeed without retry

[codecov-commenter]

Codecov Report

Merging #21087 (d96da99) into master (63d9eaf) will increase coverage by 35.90%.
Report is 61 commits behind head on master.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #21087       +/-   ##
=============================================
+ Coverage     36.92%   72.82%   +35.90%     
- Complexity    12187    32725    +20538     
=============================================
  Files          1698     1887      +189     
  Lines        129846   144180    +14334     
  Branches      14163    16491     +2328     
=============================================
+ Hits          47947   105004    +57057     
+ Misses        75570    30833    -44737     
- Partials       6329     8343     +2014     

Flag	Coverage Δ
inttests	25.07% <20.00%> (+0.92%)	⬆️
systests	25.94% <20.00%> (+0.79%)	⬆️
unittests	72.00% <100.00%> (+39.92%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...pache/pulsar/broker/admin/impl/NamespacesBase.java	72.82% <ø> (+41.52%)	⬆️
...g/apache/pulsar/client/impl/RawBatchConverter.java	93.50% <100.00%> (+19.82%)	⬆️
...rg/apache/pulsar/compaction/TwoPhaseCompactor.java	78.85% <100.00%> (+8.66%)	⬆️

... and 1454 files with indirect coverage changes