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

[improve][broker][PIP-149]Make getList async #16221

Merged
merged 5 commits into from
Jun 30, 2022
Merged

[improve][broker][PIP-149]Make getList async #16221

merged 5 commits into from
Jun 30, 2022

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Jun 26, 2022

Master Issue: #14365

Motivation

Modifications

  • Make getList async

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 26, 2022
@AnonHxy AnonHxy changed the title [PIP-149][broker]Make getList async [improve][PIP-149][broker]Make getList async Jun 26, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 26, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy changed the title [improve][PIP-149][broker]Make getList async [improve][broker][PIP-149]Make getList async Jun 26, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 26, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy changed the title [improve][broker][PIP-149]Make getList async [wip][improve][broker][PIP-149]Make getList async Jun 26, 2022
@AnonHxy AnonHxy changed the title [wip][improve][broker][PIP-149]Make getList async [improve][broker][PIP-149]Make getList async Jun 26, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 27, 2022

/pulsarbot run-failure-checks

nodece
nodece reviewed Jun 28, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated
@@ -4251,7 +4243,7 @@ private RestException topicNotFoundReason(TopicName topicName) {
== null ? "has no metadata" : "has zero partitions";
return new RestException(Status.NOT_FOUND, String.format(
"Partitioned Topic not found: %s %s", topicName.toString(), topicErrorType));
} else if (!internalGetList(Optional.empty()).contains(topicName.toString())) {
} else if (!internalGetListAsync(Optional.empty()).join().contains(topicName.toString())) {
Copy link
Member

@nodece nodece Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We need to avoid using the join() and get().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we keep both internalGetList and internalGetListAsync? @nodece

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you need to make the topicNotFoundReason to async.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has existed a method topicNotFoundReasonAsync, but topicNotFoundReason are still invoked by getTopicReference, which appears in many methods. I think it's better to cleanup topicNotFoundReason when other methods have replaced getTopicReference with getTopicReferenceAsync.So it's looks OK using join here @nodece

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it's better using get(pulsar().getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS) instead of join().

Copy link
Contributor

Choose a reason for hiding this comment

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

sync(() -> topicNotFoundReasonAsync()

nodece
nodece reviewed Jun 28, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated
@@ -4271,7 +4263,7 @@ private CompletableFuture<Topic> topicNotFoundReasonAsync(TopicName topicName) {
== null ? "has no metadata" : "has zero partitions";
throw new RestException(Status.NOT_FOUND, String.format(
"Partitioned Topic not found: %s %s", topicName.toString(), topicErrorType));
} else if (!internalGetList(Optional.empty()).contains(topicName.toString())) {
} else if (!internalGetListAsync(Optional.empty()).join().contains(topicName.toString())) {
Copy link
Member

Choose a reason for hiding this comment

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

We need to avoid using the join() and get().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nodece , I removed join in topicNotFoundReasonAsync. PTAL

nodece
nodece reviewed Jun 28, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated
return new RestException(Status.NOT_FOUND, "Topic partitions were not yet created");
}
} catch (Exception e) {
// failed to get reason, so fall through coarse reason
Copy link
Member

Choose a reason for hiding this comment

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

I think we should throw an error, or return return new RestException(Status.NOT_FOUND, e.getMessage());

You can look at other people's opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

en...It seems that e.getMessage is not the real reason I think @nodece , Please take a look also @Technoboy- @Jason918

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can add a new sync method called internalGetList like getPartitionedTopicMetadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks bettter to keep the original internalGetList, and cleanup it until making all other methods async @Technoboy-

Copy link
Contributor

Choose a reason for hiding this comment

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

In order to reduce the duplicated codes, you can do it like sync(() -> internalGetListAsync()

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 29, 2022

/pulsarbot run-failure-checks

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Jun 29, 2022

/pulsarbot run-failure-checks

@Technoboy- Technoboy- added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/admin labels Jun 29, 2022
@Technoboy- Technoboy- added this to the 2.11.0 milestone Jun 29, 2022
Jason918
Jason918 approved these changes Jun 30, 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

@Jason918 Jason918 merged commit 92d229d into apache:master Jun 30, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Jul 5, 2022
@AnonHxy
[improve][broker][PIP-149]Make getList async (apache#16221)
eba34cd
wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022
@AnonHxy @weimob-wuxuanqi
[improve][broker][PIP-149]Make getList async (apache#16221)
dc43f2f
congbobo184 pushed a commit to congbobo184/pulsar that referenced this pull request Dec 8, 2022
[cherry-pick][branch-2.9] make getList async apache#16221
deaa2d4
@congbobo184 congbobo184 mentioned this pull request Dec 8, 2022
Merged
2 tasks
Technoboy- pushed a commit that referenced this pull request Dec 8, 2022
@congbobo184
make getList async #16221 (#18811)
807c0a4 
Co-authored-by: congbobo184 <[email protected]>
@liangyepianzhou liangyepianzhou mentioned this pull request Dec 8, 2022
Merged
14 tasks
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nodece nodece nodece approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@Jason918 Jason918 Jason918 approved these changes

Assignees

@AnonHxy AnonHxy

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

Successfully merging this pull request may close these issues.
7 participants
@AnonHxy @Jason918 @Technoboy- @nodece @codelipenghui @congbobo184 @liangyepianzhou