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.

Sign up for GitHub

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

Jump to bottom

fix delete authentication policies when delete topic. #12215

Merged
merged 13 commits into from
Oct 27, 2021
Merged

fix delete authentication policies when delete topic. #12215

merged 13 commits into from
Oct 27, 2021

Conversation

LeBW
Copy link
Contributor

@LeBW LeBW commented Sep 28, 2021

Fixes #11876

Motivation

When delete the topic, its authentication policies in zookeeper are not deleted.

Modifications

delete the authentication policies of the topic when delete the topic.

Verifying this change

This change added tests and can be verified as follows:

  • Added unit test.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): ( no)
  • The public API: (no)
  • The schema: ( no )
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

bug fix

@michaeljmarshall
Copy link
Member

@eolivelli - PTAL

@LeBW
Copy link
Contributor Author

LeBW commented Sep 28, 2021

/pulsarbot run-failure-checks

@codelipenghui codelipenghui added this to the 2.9.0 milestone Sep 29, 2021
@codelipenghui codelipenghui added release/2.8.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Sep 29, 2021
codelipenghui
codelipenghui requested changes Sep 29, 2021
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/DeleteAuthenticationPoliciesTest.java Outdated
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

public class DeleteAuthenticationPoliciesTest extends MockedPulsarServiceBaseTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test group?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we already have AuthenticatedProducerConsumerTest there, it's better to move the test to AuthenticatedProducerConsumerTest which will bring convenience to test maintenance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix it.

pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/DeleteAuthenticationPoliciesTest.java Outdated

private static void waitForChange() {
try {
Thread.sleep(100);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid sleep, it's better to use Awaitbility to instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix it

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Sep 30, 2021
@eolivelli eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021
hangc0276
hangc0276 reviewed Oct 11, 2021
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated
.deleteTopicAuthenticationWithRetry(topicName.toString(), deleteTopicAuthenticationFuture, 5);
deleteTopicAuthenticationFuture.whenComplete((r1, ex1) -> {
if (ex1 != null) {
asyncResponse.resume(new RestException(ex1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add error log for the authentication delete failed exception.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved
@LeBW
Copy link
Contributor Author

LeBW commented Oct 21, 2021

/pulsarbot run-failure-checks

@gaoran10
Copy link
Contributor

gaoran10 commented Oct 25, 2021 •
edited
Loading

@hangc0276 @codelipenghui @michaeljmarshall All comments were fixed, could you take a look again?

gaoran10
gaoran10 approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

codelipenghui
codelipenghui approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check @315157973's comment.

@codelipenghui codelipenghui merged commit 3e57828 into apache:master Oct 27, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 27, 2021
Merge branch 'up/master' into website/branch-2.7.2-chapter-4
af0565e 
* up/master:
  fix delete authentication policies when delete topic. (apache#12215)
  [Broker] Fix messageDedup delete inactive producer name (apache#12493)
  Fixed getting children of parent nodes in LocalMemoryMetadataStore (apache#12491)
  Update Producer stats on producer close() (apache#12500)
  docs(cli):add restart command in pulsar-daemon (apache#12373)
  Add the pulsar java property memory allocator doc (apache#12481)
  [Doc]Update ci-documentbot.yml (apache#12480)
@codelipenghui
Copy link
Contributor

@LeBW Could you please help create a PR to branch-2.8 directly? we are not able to cherry-pick this one directly because it depends on #11693

hangc0276 pushed a commit that referenced this pull request Nov 4, 2021
@LeBW @hangc0276
fix delete authentication policies when delete topic. (#12215)
c005a42 
(cherry picked from commit 3e57828)
@hangc0276
Copy link
Contributor

@LeBW Could you please help create a PR to branch-2.8 directly? we are not able to cherry-pick this one directly because it depends on #11693

I have fixed the cherry-pick issue in ffe574d , Please help review this commit, thanks. @LeBW @codelipenghui

@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Nov 4, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
@LeBW @eolivelli
fix delete authentication policies when delete topic. (apache#12215)
d115c17
codelipenghui pushed a commit that referenced this pull request Dec 20, 2021
@LeBW @codelipenghui
fix delete authentication policies when delete topic. (#12215)
f1d8d71 
(cherry picked from commit 3e57828)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangc0276 hangc0276 hangc0276 left review comments

@michaeljmarshall michaeljmarshall michaeljmarshall left review comments

@315157973 315157973 315157973 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@LeBW LeBW

Labels
area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2 release/2.9.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

在topic层级进行produce、consume授权后，zk上的权限数据一直堆积无法被删除
8 participants
@LeBW @michaeljmarshall @gaoran10 @codelipenghui @hangc0276 @315157973 @eolivelli @Anonymitaet