[pulsar-broker] support to get list of topics under a namespace bundle #12632
Conversation
rdhabalia
requested review from
merlimat,
sijie,
jiazhai,
eolivelli,
codelipenghui and
massakam
|
@rdhabalia:Thanks for your contribution. For this PR, do we need to update docs? |
rdhabalia
force-pushed
the
list_bundle
branch
from
f3b451c
to
8a35c6f
Compare
Anonymitaet
added
doc
|
@rdhabalia do we need to add docs here? |
| * @throws PulsarAdminException | ||
| * Unexpected error | ||
| */ | ||
| List<String> getList(String namespace, String bundle, TopicDomain topicDomain) throws PulsarAdminException; |
There was a problem hiding this comment.
Suggests to provide the corresponding getListAsync method like the others?
There was a problem hiding this comment.
How about introducing a GetListOption? we might want to add more params in the future, this will make the API looks very bloated
There was a problem hiding this comment.
added getListAsync with param map for future extension.
| @@ -889,7 +889,7 @@ public void topics() throws Exception { | |||
| verify(mockTopics).revokePermissions("persistent://myprop/clust/ns1/ds1", "admin"); | |||
|
|
|||
| cmdTopics.run(split("list myprop/clust/ns1")); | |||
| verify(mockTopics).getList("myprop/clust/ns1", null); | |||
| verify(mockTopics).getList("myprop/clust/ns1", null, null); | |||
There was a problem hiding this comment.
It's better to keep the previous code line as the interface is not removed.
There was a problem hiding this comment.
we are not removing the code but CLI admin list command now uses the new interface which this test is validating.
| * @throws PulsarAdminException | ||
| * Unexpected error | ||
| */ | ||
| List<String> getList(String namespace, TopicDomain topicDomain, Map<String, Object> params) |
There was a problem hiding this comment.
IMHO, It's a little confusing to add params here. It's easy for coding, but not so easy for user to understand how to use this.
There was a problem hiding this comment.
yes, added an enum to use query param for usability.
rdhabalia
force-pushed
the
list_bundle
branch
3 times, most recently
from
3e08154
to
6867253
Compare
|
@eolivelli @Jason918 PTAL. |
|
ping |
|
this PR again has conflicts. can we please review and merge the PR. |
|
@Jason918 Please help review the PR again. |
|
@rdhabalia Please help resolve the conflicts so that we can merge this PR. |
|
@Jason918 I have resolved the conflicts, please review again. |
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
|
@rdhabalia Could you please help check the failed test? Looks like related to this PR. |
rdhabalia
force-pushed
the
list_bundle
branch
from
0b51aaf
to
91e26b8
Compare
add param methods add query param enum
rdhabalia
force-pushed
the
list_bundle
branch
from
91e26b8
to
af9fc87
Compare
apache#12632) add param methods add query param enum
Motivation
Right now, broker-admin api allows user to get list of topics under a namespace but sometime user also needs a list of topics under a given namespace bundle. so, add support to fetch list of topics under a namespace
Modificaiton
Result