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] Make deleteTopicPolicies serialized is executed when close topic. #15811

Merged
merged 3 commits into from
Aug 22, 2022
Merged

[fix][broker] Make deleteTopicPolicies serialized is executed when close topic. #15811

merged 3 commits into from
Aug 22, 2022

Conversation

mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented May 27, 2022

Motivation

In the current implementation, when closing the topic we use thenAccept to delete topic policies. It will ignore the exception and do not waiting for the inner CompletableFuture.

Modifications

  • Use thenCompose to instead of thenAccept
  • When namespace is marked deleted, we don't need to delete topic policies.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-not-needed
    (Please explain why)

@Technoboy- Technoboy- added this to the 2.11.0 milestone May 27, 2022
@mattisonchao mattisonchao reopened this Jun 1, 2022
@nodece
Copy link
Member

nodece commented Jun 12, 2022

Please rebase this PR.

@codelipenghui
Copy link
Contributor

Looks like the change breaks lots of the tests, move to 2.10.2 first

@HQebupt
Copy link
Contributor

HQebupt commented Jul 5, 2022

/pulsarbot run-failure-checks

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@BewareMyPower
Copy link
Contributor

/pulsarbot run-failure-checks

@mattisonchao mattisonchao changed the title [fix][broker] Avoid using thenAccept invoke async method. [fix][broker] Make deleteTopicPolicies serialized is executed when close topic. Aug 19, 2022
@mattisonchao mattisonchao reopened this Aug 19, 2022
@@ -80,6 +80,7 @@ protected void setup() throws Exception {
conf.setTlsCertificateFilePath(TLS_SERVER_CERT_FILE_PATH);
conf.setTlsKeyFilePath(TLS_SERVER_KEY_FILE_PATH);
conf.setTlsAllowInsecureConnection(true);
conf.setTopicLevelPoliciesEnabled(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Because TLS is not configured for the internal Pulsar client, we get an exception when we delete the topic policy.

I'm wondering if we enable topicPolicies and TLS but don't provide any TLS configuration for the broker's internal clients.
Do we have to check the broker`s internal client configuration when the broker starts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattisonchao Please open a new issue for this one.

Comment on lines +2935 to +2936
return pulsarService.getPulsarResources().getNamespaceResources()
.getPoliciesAsync(topicName.getNamespaceObject())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to get the namespace policy first?

Copy link
Member Author

@mattisonchao mattisonchao Aug 22, 2022

Choose a reason for hiding this comment

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

We want to check if the namespace is marked as "deleted". This means the namespace is being deleted. We don't need to delete the topic policy anymore.
If we connect to the deleted namespace. we will get an error.

@codelipenghui codelipenghui merged commit e8ee996 into apache:master Aug 22, 2022
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Aug 22, 2022
Technoboy- pushed a commit that referenced this pull request Aug 22, 2022
congbobo184 pushed a commit that referenced this pull request Nov 19, 2022
@congbobo184 congbobo184 added release/2.9.4 cherry-picked/branch-2.9 Archived: 2.9 is end of life labels Nov 19, 2022
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.9.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants