-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[improve][cli][PIP-280] Retrofit pulsar-cli-utils
into pulsar-broker
and pulsar-client-tools
#21412
Conversation
/cc @tisonkun, WDYT? |
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.
Out thread - I found that RelativeTimeUtil
is duplicated in pulsar-common
and pulsar-cli-utils
. What's the reason, and if we can eliminate the duplicate?
WRT failing tests, they are flakey since they all pass in my fork. |
Indeed, The rationale is that the duplication serves as a small trade-off to avoid introducing a dependency between the two modules. Furthermore, the likelihood of changes to the |
Lari fixes an amount of flaky tests these days, you can merge master and push this branch to retrigger the CI. |
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.
Merging.. |
…sar-broker` and `pulsar-client-tools` (apache#21412)" This reverts commit 36d4708.
pulsar-cli-utils
module #21056Motivation
pulsar-cli-utils
intopulsar-broker
andpulsar-client-tools
.Modifications
TokensCliUtils
inpulsar-broker
module to delegate time parsing logic topulsar-cli-utils
.CmdPersistentTopics
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: https://github.com/JooHyukKim/pulsar/pull/23/