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 call sync method in async rest API for internalGetSubscriptionsForNonPartitionedTopic #13745

Merged
merged 12 commits into from Jan 18, 2022
Merged

Fix call sync method in async rest API for internalGetSubscriptionsForNonPartitionedTopic #13745

merged 12 commits into from Jan 18, 2022

Conversation

mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Jan 13, 2022 •
edited

Motivation

Avoid call sync method in async rest API for PersistentTopicsBase#internalGetSubscriptionsForNonPartitionedTopic.

Modifications

  • Use async instead of sync method.

Verifying this change

  • Make sure that the change passes the CI checks.

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

  • 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

  • no-need-doc

@mattisonchao mattisonchao changed the title Fix call sync method in async rest API for internalGetSubscriptionsForNonPartitionedTopic s Fix call sync method in async rest API for internalGetSubscriptionsForNonPartitionedTopics Jan 13, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 13, 2022
@codelipenghui codelipenghui added this to the 2.10.0 milestone Jan 13, 2022
@codelipenghui codelipenghui added release/2.8.3 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug labels Jan 13, 2022
@mattisonchao
Copy link
Member Author

Thanks @codelipenghui,
I need to be more careful to avoid this kind of problem.

PTAL :)

@mattisonchao mattisonchao reopened this Jan 14, 2022
Jason918
Jason918 approved these changes Jan 15, 2022
View reviewed changes
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

@mattisonchao mattisonchao changed the title Fix call sync method in async rest API for internalGetSubscriptionsForNonPartitionedTopics Fix call sync method in async rest API for internalGetSubscriptionsForNonPartitionedTopic Jan 15, 2022
@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

mattisonchao and others added 11 commits January 17, 2022 12:01
@mattisonchao
Fix async method call sync method.
7593ad5
@mattisonchao @codelipenghui
Update pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/imp…
4e42b14 
…l/PersistentTopicsBase.java

Co-authored-by: lipenghui <penghui@apache.org>
@mattisonchao @codelipenghui
Update pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/imp…
207c88e 
…l/PersistentTopicsBase.java

Co-authored-by: lipenghui <penghui@apache.org>
@mattisonchao @codelipenghui
Update pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/imp…
4e88508 
…l/PersistentTopicsBase.java

Co-authored-by: lipenghui <penghui@apache.org>
@mattisonchao @codelipenghui
Update pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/imp…
2c8eaef 
…l/PersistentTopicsBase.java

Co-authored-by: lipenghui <penghui@apache.org>
@mattisonchao
fix wrong instanceof judgement.
63cde59
@mattisonchao
Print error log when WebApplicationException is not TEMPORARY_REDIRECT
ee59925
@mattisonchao
Add validateTopicOperationAsync method.
16a91ca
@mattisonchao
Fixes unhanlde restexception
6647951
@mattisonchao
Fixes async method effect test issue.
2b5df1b 
- Due to async method do not hanlde ``NOT_ALLOWED_EXCEPTION``.
@mattisonchao
Fix checkstyle
0d7b555
@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

codelipenghui
codelipenghui reviewed Jan 17, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java Outdated
validateTopicOperationAsync(topicName, operation, subscription).get();
} catch (InterruptedException | ExecutionException e) {
Throwable cause = e.getCause();
log.error("Validate Topic [%s] operation [%s] got server internal error.", cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct the log content.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed it. :)
Since we're throwing a new exception below, I don't think we need to log here anymore.

@mattisonchao mattisonchao reopened this Jan 17, 2022
@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

@codelipenghui codelipenghui merged commit 0a046b9 into apache:master Jan 18, 2022
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
@mattisonchao @codelipenghui
Fix call sync method in async rest API for ``internalGetSubscriptions…
f36b701 
…ForNonPartitionedTopic`` (#13745)

(cherry picked from commit 0a046b9)
@codelipenghui codelipenghui added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Jan 18, 2022
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
@mattisonchao @codelipenghui
Fix call sync method in async rest API for ``internalGetSubscriptions…
f9aae1f 
…ForNonPartitionedTopic`` (#13745)

(cherry picked from commit 0a046b9)
@lhotari lhotari mentioned this pull request Feb 7, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jason918 Jason918 Jason918 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

Assignees

@mattisonchao mattisonchao

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.3 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mattisonchao @Jason918 @codelipenghui @gaoran10