-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[feat][broker] PIP-452: Customizable topic listing of namespace with properties #25134
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
base: master
Are you sure you want to change the base?
Conversation
06e3b36 to
7348e97
Compare
pip/pip-452.md
Outdated
| .thenApply(topics -> TopicListingResult.success(topics, false)); | ||
| } | ||
| }).thenCompose(listingResult -> { | ||
| List<String> rawTopics = listingResult.getTopics(); |
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.
You don't have to copy so much code here, which does not focus on the key change. The only thing that matters is that what will replace the following existing code:
return getBrokerService().pulsar().getNamespaceService()
.getListOfUserTopics(namespaceName, mode)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.
Simplified pseudocode
pip/pip-452.md
Outdated
| * Return an invalid result indicating that the caller should continue with the default logic. | ||
| */ | ||
| public static TopicListingResult passThrough() { | ||
| return PASS_THROUGH_INSTANCE; |
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.
This design is a bit over-abstraction. IMO, the following design could be simpler and clearer.
public record TopicListingResult(List<String> topics, boolean filtered) {} /**
* @return the future of the result, if it's empty, fall back to the built-in implementation to list all topics
*/
default CompletableFuture<Optional<TopicListingResult>> interceptGetTopicsOfNamespace(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.
Since the built-in implementation is very simple that calls method directly on the NamespaceService, I think the fallback logic is unnecessary.
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.
Since we allow the configuration of multiple interceptors, the return here means that the result of current interception is not processed this method. If all the interceptors do not process the method, the default logic will be used.
In addition, the interception configuration is empty by default. In this case, we still need to fall back to the default logic.
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.
default CompletableFuture<Optional<TopicListingResult>> interceptGetTopicsOfNamespace()
Applied this suggestion
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.
Pull request overview
This PR introduces PIP-452, which proposes a design for customizable topic listing in Pulsar's GetTopicsOfNamespace command. The proposal aims to make topic discovery more flexible by allowing clients to pass context properties and enabling plugins to override the default metadata store scanning behavior.
Changes:
- Adds protocol extension to include properties field in CommandGetTopicsOfNamespace
- Introduces BrokerInterceptor interface method for custom topic listing logic
- Extends REST API and CLI to support properties parameter for topic listing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cfa592c to
70224d9
Compare
…space with properties
…operties in ListTopicsOptions
…st watch is not supported.
Fixes #xyz
Main Issue: #xyz
PIP: #xyz
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: