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

[Broker] Adjust topic exists check logic in http lookup process #14199

Merged

Conversation

gaoran10
Copy link
Contributor

@gaoran10 gaoran10 commented Feb 10, 2022

Motivation

Currently, it's hard to check the non-persistent-non-partitioned topic exists or not, because it only exists in the broker cache, it doesn't have metadata.

If there is a non-persistent and non-partitioned topic exists in broker0 and the configuration AllowAutoTopicCreation is false, we perform an HTTP lookup request to broker1, it will return a TopicNotFound exception.

Modifications

If the topic is non-persistent and non-partitioned, we'll return the true flag to ensure the http lookup can return a result.

Verifying this change

Add a test to verify the HTTP lookup request to each broker could return a right lookup result.

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 or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@gaoran10 gaoran10 self-assigned this Feb 10, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 10, 2022
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM. Good catch.

I think we can go further and just skip this exist check if pulsar().getBrokerService().isAllowAutoTopicCreation(topicName) is true?

@codelipenghui codelipenghui added this to the 2.10.0 milestone Feb 10, 2022
@codelipenghui
Copy link
Contributor

The related change introduced by #13055 and only for the master branch, so don't need to cherry-pick to other branches.
And this change is a 2.10 blocker, otherwise, we will introduce breaking change in 2.10.

@codelipenghui codelipenghui added release/blocker Indicate the PR or issue that should block the release until it gets resolved and removed release/2.9.3 labels Feb 10, 2022
@Technoboy-
Copy link
Contributor

LGTM. Good catch.

I think we can go further and just skip this exist check if pulsar().getBrokerService().isAllowAutoTopicCreation(topicName) is true?

Seems a good idea. @codelipenghui @gaoran10

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

LGTM.
But the idea that @Jason918 commented looks perfect.

@gaoran10
Copy link
Contributor Author

@Jason918 @Technoboy- Thanks, I improve the logic, please check the last commit.

Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

Great.

@codelipenghui codelipenghui merged commit ee5f6c0 into apache:master Feb 10, 2022
@gaoran10 gaoran10 deleted the gaoran/adjust-lookup-topic-check branch February 10, 2022 15:23
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…he#14199)

### Motivation

Currently, it's hard to check the non-persistent-non-partitioned topic exists or not, because it only exists in the broker cache, it doesn't have metadata.

If there is a non-persistent and non-partitioned topic exists in broker0 and the configuration `AllowAutoTopicCreation` is false, we perform an HTTP lookup request to broker1, it will return a TopicNotFound exception.

### Modifications

If the topic is non-persistent and non-partitioned, we'll return the true flag to ensure the http lookup can return a result.

### Verifying this change

Add a test to verify the HTTP lookup request to each broker could return a right lookup result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs release/blocker Indicate the PR or issue that should block the release until it gets resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants