[fix][broker] Fix can not delete namespace by force

[liangyepianzhou]

Motivation

1. When we delete a namespace by force if the __transaction_buffer_snapshot and __change_events be deleted first, and then the deleting of the normal topic will be failed at clearing the snapshot and clearing topic policies.
2. fix flaky test testCreateTransactionSystemTopic. Test failed due to cleaning up after class.

Modification

1. Delete the normal topic and then delete the system topic.
2. The system topic does not need to clean up topic policies.

For modification 2, there is a supported error msg.

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

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
• Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: liangyepianzhou#8

[codecov-commenter]

Codecov Report

Merging #18307 (a5ef7a3) into master (67d9d63) will increase coverage by 6.73%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master   #18307      +/-   ##
============================================
+ Coverage     40.29%   47.02%   +6.73%     
- Complexity     8685    10369    +1684     
============================================
  Files           687      692       +5     
  Lines         67441    67779     +338     
  Branches       7225     7260      +35     
============================================
+ Hits          27175    31876    +4701     
+ Misses        37257    32322    -4935     
- Partials       3009     3581     +572     

Flag	Coverage Δ
unittests	47.02% <87.50%> (+6.73%)	⬆️

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

Impacted Files	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	36.87% <0.00%> (-0.03%)	⬇️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	62.80% <100.00%> (-1.10%)	⬇️
...sar/broker/service/persistent/PersistentTopic.java	61.66% <100.00%> (-0.09%)	⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java	61.11% <0.00%> (-16.67%)	⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java	54.12% <0.00%> (-13.77%)	⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java	40.74% <0.00%> (-12.35%)	⬇️
...apache/pulsar/broker/service/TopicListService.java	42.62% <0.00%> (-12.30%)	⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java	71.72% <0.00%> (-6.29%)	⬇️
...rg/apache/pulsar/broker/web/PulsarWebResource.java	53.79% <0.00%> (-4.66%)	⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java	60.66% <0.00%> (-4.21%)	⬇️
... and 207 more

[liangyepianzhou]

@Technoboy- Why the namespace should not be deleted here?

pulsar/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionProduceTest.java

Lines 93 to 119 in d54c2cb

	public void testDeleteNamespaceBeforeCommit() throws Exception {
	final String topic = NAMESPACE1 + "/testDeleteTopicBeforeCommit";
	PulsarClient pulsarClient = this.pulsarClient;
	Transaction tnx = pulsarClient.newTransaction()
	.withTransactionTimeout(60, TimeUnit.SECONDS)
	.build().get();
	long txnIdMostBits = ((TransactionImpl) tnx).getTxnIdMostBits();
	long txnIdLeastBits = ((TransactionImpl) tnx).getTxnIdLeastBits();
	Assert.assertTrue(txnIdMostBits > -1);
	Assert.assertTrue(txnIdLeastBits > -1);
	@Cleanup
	Producer<byte[]> outProducer = pulsarClient
	.newProducer()
	.topic(topic)
	.sendTimeout(0, TimeUnit.SECONDS)
	.enableBatching(false)
	.create();
	String content = "Hello Txn";
	outProducer.newMessage(tnx).value(content.getBytes(UTF_8)).send();
	try {
	deleteNamespaceGraceFully(NAMESPACE1, true);
	} catch (Exception ignore) {}
	tnx.commit().get();
	}

[poorbarcode]

Good catch. left some comments.

When we delete a namespace by force if the __transaction_buffer_snapshot and __change_events be deleted first, and then the deleting of the normal topic will be failed at clearing the snapshot and clearing topic policies.

Can we add a test for this case?

[Technoboy-]

@mattisonchao Could you help review this?

[codelipenghui]

Why we should delete the entire test?
If the expected behavior is the transaction is not allowed to commit. We can just change the test to check the expected exception.

[liangyepianzhou]

This test is strange. The expected behavior is the namespace can be deleted by force when
there are transactions used to send a message. But the test is executed in the opposite logic.

[Technoboy-]

@Technoboy- Why the namespace should not be deleted here?

pulsar/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionProduceTest.java

Lines 93 to 119 in d54c2cb

	public void testDeleteNamespaceBeforeCommit() throws Exception {
	final String topic = NAMESPACE1 + "/testDeleteTopicBeforeCommit";
	PulsarClient pulsarClient = this.pulsarClient;
	Transaction tnx = pulsarClient.newTransaction()
	.withTransactionTimeout(60, TimeUnit.SECONDS)
	.build().get();
	long txnIdMostBits = ((TransactionImpl) tnx).getTxnIdMostBits();
	long txnIdLeastBits = ((TransactionImpl) tnx).getTxnIdLeastBits();
	Assert.assertTrue(txnIdMostBits > -1);
	Assert.assertTrue(txnIdLeastBits > -1);
	@Cleanup
	Producer<byte[]> outProducer = pulsarClient
	.newProducer()
	.topic(topic)
	.sendTimeout(0, TimeUnit.SECONDS)
	.enableBatching(false)
	.create();
	String content = "Hello Txn";
	outProducer.newMessage(tnx).value(content.getBytes(UTF_8)).send();
	try {
	deleteNamespaceGraceFully(NAMESPACE1, true);
	} catch (Exception ignore) {}
	tnx.commit().get();
	}

Should be related with #14991. There is one case that the test did not coverage after discuss then. So add this test. Do you remember the case ? @codelipenghui @congbobo184 @mattisonchao

[codelipenghui]

@liangyepianzhou Could you please help push a PR to fix branch-2.10, branch-2.9, and branch-2.11? I tried to cherry-pick it directly, but it looks like there are many conflicts. It's better to create a PR to get more eyes on the change to release branches.

[nicoloboschi]

In case you want to delete the namespace and this fix is not on the broker, the workaround is to manually delete the topics in the namespace created by the users and then trigger the namespace deletion again, right @liangyepianzhou ?

[liangyepianzhou]

@nicoloboschi If they use the topic of topic policy. Then the fix has to be contained in the broker.
There is also a problem that the topic of topic policy cannot be successfully deleted.

[Technoboy-]

Cherry-picked by #18826