Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Fix delete system topic clean topic policy #18823

Merged
merged 6 commits into from Dec 9, 2022
Merged

[fix][broker] Fix delete system topic clean topic policy #18823

merged 6 commits into from Dec 9, 2022

Conversation

liangyepianzhou
Copy link
Contributor

Motivation

If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

Modification

Only change_events topic do not need to clear topic policies.

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

### Motivation
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

### Modification
Only change_events topic do not need to clear topic policies.
@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2022

Codecov Report

Merging #18823 (82d5817) into master (da87e40) will increase coverage by 2.98%.
The diff coverage is 70.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18823      +/-   ##
============================================
+ Coverage     46.42%   49.41%   +2.98%     
- Complexity    10437    10853     +416     
============================================
  Files           703      703              
  Lines         68816    68828      +12     
  Branches       7377     7377              
============================================
+ Hits          31950    34011    +2061     
+ Misses        33252    31131    -2121     
- Partials       3614     3686      +72     
Flag Coverage Δ
unittests 49.41% <70.00%> (+2.98%) ⬆️

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

Impacted Files Coverage Δ
...ache/pulsar/broker/loadbalance/LinuxInfoUtils.java 33.72% <0.00%> (+5.81%) ⬆️
...ker/resourcegroup/ResourceQuotaCalculatorImpl.java 4.16% <0.00%> (ø)
...pache/pulsar/broker/admin/impl/NamespacesBase.java 64.44% <83.33%> (+1.39%) ⬆️
...ker/loadbalance/impl/LinuxBrokerHostUsageImpl.java 79.68% <100.00%> (+0.32%) ⬆️
.../service/SystemTopicBasedTopicPoliciesService.java 72.56% <100.00%> (+8.14%) ⬆️
...sar/broker/service/persistent/PersistentTopic.java 61.88% <100.00%> (+0.48%) ⬆️
.../pulsar/broker/service/SharedConsumerAssignor.java 3.70% <0.00%> (-74.08%) ⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java 0.00% <0.00%> (-40.75%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 47.05% <0.00%> (-13.73%) ⬇️
...ervice/persistent/MessageRedeliveryController.java 56.09% <0.00%> (-9.76%) ⬇️
... and 118 more

@poorbarcode
Copy link
Contributor

Link to

@congbobo184 congbobo184 merged commit 93c41de into apache:master Dec 9, 2022
congbobo184 pushed a commit to congbobo184/pulsar that referenced this pull request Dec 9, 2022
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

Only change_events topic do not need to clear topic policies.

(cherry picked from commit 93c41de)
liangyepianzhou added a commit that referenced this pull request Dec 9, 2022
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

Only change_events topic do not need to clear topic policies.

(cherry picked from commit 93c41de)
congbobo184 added a commit that referenced this pull request Dec 10, 2022
…18823) (#18831)

cherry-pick #18823
### Motivation
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

### Modification
Only change_events topic do not need to clear topic policies.

### Matching PR in forked repository

PR in forked repository: liangyepianzhou#16
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 10, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
### Motivation
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

### Modification
Only change_events topic do not need to clear topic policies.
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
### Motivation
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

### Modification
Only change_events topic do not need to clear topic policies.
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

Only change_events topic do not need to clear topic policies.

(cherry picked from commit 93c41de)
(cherry picked from commit 5ca889f)
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
### Motivation
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

### Modification
Only change_events topic do not need to clear topic policies.
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

Only change_events topic do not need to clear topic policies.

(cherry picked from commit 93c41de)
(cherry picked from commit 5ca889f)
poorbarcode added a commit that referenced this pull request Mar 9, 2023
Motivation: After PR #18823, The cmd delete topic will fail if disabled the feature system topic.
Modifications: do not delete the system policy if disabled the feature system topic
Technoboy- pushed a commit that referenced this pull request Mar 13, 2023
Motivation: After PR #18823, The cmd delete topic will fail if disabled the feature system topic.
Modifications: do not delete the system policy if disabled the feature system topic
liangyepianzhou added a commit that referenced this pull request Mar 16, 2023
If users set topic policy for system topic, then delete this system topic, the topic policy should be deleted.

Only change_events topic do not need to clear topic policies.

(cherry picked from commit 93c41de)
@Technoboy-
Copy link
Contributor

Cherry-picked by #19835

Technoboy- added a commit that referenced this pull request Mar 20, 2023
poorbarcode added a commit that referenced this pull request May 6, 2023
Motivation: After PR #18823, The cmd delete topic will fail if disabled the feature system topic.
Modifications: do not delete the system policy if disabled the feature system topic
(cherry picked from commit 401fb05)
coderzc pushed a commit that referenced this pull request May 31, 2023
Motivation: After PR #18823, The cmd delete topic will fail if disabled the feature system topic.
Modifications: do not delete the system policy if disabled the feature system topic

(cherry picked from commit 401fb05)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants