pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

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

Open liangyepianzhou opened this issue 3 years ago • 2 comments

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

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
  • [x] doc-not-needed
  • [ ] doc-complete

Matching PR in forked repository

PR in forked repository: https://github.com/liangyepianzhou/pulsar/pull/8

liangyepianzhou avatar Nov 02 '22 14:11 liangyepianzhou

Codecov Report

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

Impacted file tree graph

@@             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%) :arrow_up:

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%) :arrow_down:
...pache/pulsar/broker/admin/impl/NamespacesBase.java 62.80% <100.00%> (-1.10%) :arrow_down:
...sar/broker/service/persistent/PersistentTopic.java 61.66% <100.00%> (-0.09%) :arrow_down:
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) :arrow_down:
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) :arrow_down:
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-12.35%) :arrow_down:
...apache/pulsar/broker/service/TopicListService.java 42.62% <0.00%> (-12.30%) :arrow_down:
...pulsar/broker/service/PulsarCommandSenderImpl.java 71.72% <0.00%> (-6.29%) :arrow_down:
...rg/apache/pulsar/broker/web/PulsarWebResource.java 53.79% <0.00%> (-4.66%) :arrow_down:
...oker/service/schema/SchemaRegistryServiceImpl.java 60.66% <0.00%> (-4.21%) :arrow_down:
... and 207 more

codecov-commenter avatar Nov 03 '22 02:11 codecov-commenter

@Technoboy- Why the namespace should not be deleted here? https://github.com/apache/pulsar/blob/d54c2cb5f04bfb15e8b8896f49faa81130b70c29/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionProduceTest.java#L93-L119

liangyepianzhou avatar Nov 03 '22 03:11 liangyepianzhou

@mattisonchao Could you help review this?

Technoboy- avatar Nov 08 '22 08:11 Technoboy-

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

https://github.com/apache/pulsar/blob/d54c2cb5f04bfb15e8b8896f49faa81130b70c29/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionProduceTest.java#L93-L119

Should be related with https://github.com/apache/pulsar/pull/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

Technoboy- avatar Nov 18 '22 03:11 Technoboy-

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

codelipenghui avatar Nov 29 '22 14:11 codelipenghui

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 ?

nicoloboschi avatar Dec 07 '22 13:12 nicoloboschi

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

liangyepianzhou avatar Dec 07 '22 14:12 liangyepianzhou

Cherry-picked by https://github.com/apache/pulsar/pull/18826

Technoboy- avatar Mar 06 '23 02:03 Technoboy-