-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[feature][admin] Support to get topic properties. #15944
[feature][admin] Support to get topic properties. #15944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Left some small suggestions.
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
As apache#12818 has supported creating topics with metadata, this patch is adding a `get` API to support getting topic properties. (cherry picked from commit 1ebe4ee) (cherry picked from commit 6648f99)
throw new RestException(Status.NOT_FOUND, | ||
getTopicNotFoundErrorMessage(topicName.toString())); | ||
} | ||
return ((PersistentTopic) opt.get()).getManagedLedger().getProperties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/apache/pulsar/pull/12818/files introduced ManagedLedgerConfig.properties
https://github.com/apache/pulsar/pull/12818/files#diff-c917471bc697b80ff5e8eb5de212597c527e756ea49694c7fbbabacbf2b9752cR76.
However, we are returning ManagedLedgerImpl.propertiesMap here.
Why do we have two properties in two different places?
Motivation
As #12818 has supported creating topics with metadata, this patch is adding a
get
API to support getting topic properties.Documentation
doc-not-needed
(Please explain why)