[refactor][cli][PIP-280] Create new `pulsar-cli-utils` module #20782

JooHyukKim · 2023-07-11T15:23:42Z

PIP: #20691

Motivation

As PIP-280 #20691 may span cross module, we need a module to provide CLI related (though mostly JCommander atm) isolation. Note that pulsar-common was not considered because it's common (too broad).

Modifications

Add new module pulsar-cli-utils
Implement Converters for meaurement unit (time,byte) conversion
Implement Validators for

Note

This PR blocks [refactor][cli][PIP-280] Refactor pulsar-client-tools module #20764

Verifying this change

Make sure that the change passes the CI checks.

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/19

codecov-commenter · 2023-07-12T06:36:05Z

Codecov Report

Merging #20782 (d9d2194) into master (9b6a123) will increase coverage by 0.52%.
The diff coverage is 75.86%.

@@             Coverage Diff              @@
##             master   #20782      +/-   ##
============================================
+ Coverage     72.61%   73.14%   +0.52%     
- Complexity    31966    32240     +274     
============================================
  Files          1868     1880      +12     
  Lines        139185   139300     +115     
  Branches      15315    15323       +8     
============================================
+ Hits         101071   101891     +820     
+ Misses        30052    29353     -699     
+ Partials       8062     8056       -6

Flag	Coverage Δ
inttests	`24.12% <77.27%> (?)`
systests	`25.29% <77.27%> (+0.13%)`	⬆️
unittests	`72.43% <75.86%> (+0.45%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...apache/pulsar/cli/converters/RelativeTimeUtil.java	`28.57% <28.57%> (ø)`
...org/apache/pulsar/cli/converters/ByteUnitUtil.java	`58.82% <58.82%> (ø)`
...lsar/cli/converters/TimeUnitToMillisConverter.java	`71.42% <71.42%> (ø)`
...sar/cli/converters/TimeUnitToSecondsConverter.java	`71.42% <71.42%> (ø)`
...va/org/apache/pulsar/cli/ValueValidationUtils.java	`93.75% <93.75%> (ø)`
.../apache/pulsar/broker/resources/BaseResources.java	`72.72% <95.23%> (+5.31%)`	⬆️
...he/pulsar/broker/resources/NamespaceResources.java	`81.42% <100.00%> (ø)`
...ulsar/cli/converters/ByteUnitIntegerConverter.java	`100.00% <100.00%> (ø)`
...pulsar/cli/converters/ByteUnitToLongConverter.java	`100.00% <100.00%> (ø)`
...r/cli/validators/IntegerMaxValueLongValidator.java	`100.00% <100.00%> (ø)`
... and 4 more

... and 122 files with indirect coverage changes

JooHyukKim · 2023-07-12T10:50:06Z

/pulsarbot rerun-failure-checks

JooHyukKim · 2023-07-12T10:52:15Z

Re-running the following failed checks

OWASP dependency check : seems like it has to with rate limiting.
Pulsar IO - Oracle Browser just freezes, maybe too much log? (I will check other completed Actions to verify)

EDIT : Made a PR in to cover OWASP check #20792

…odule

JooHyukKim · 2023-07-13T02:48:15Z

Need #20792 to resolve the OWASP blocker

…odule

JooHyukKim · 2023-07-16T11:13:25Z

May I ask for a review when you have time 😆, @tisonkun @BewareMyPower @mattisonchao ? thanks 🙏🏼

JooHyukKim · 2023-07-19T01:15:19Z

/pulsarbot rerun-failure-checks

…odule

JooHyukKim · 2023-07-19T02:17:30Z

#20792 should fixs the OWASP check currently failing, but the suppression turns out to be more difficult than it seems. Any help will be welcome

tisonkun · 2023-07-20T10:25:49Z

@JooHyukKim you can now rebase this branch on the fixed master :D

…odule

pulsar-cli-utils/pom.xml

…odule

pulsar-cli-utils/src/main/java/org/apache/pulsar/cli/converters/RelativeTimeUtil.java

gaoran10

Great work! LGTM

Co-authored-by: ran <[email protected]>

BewareMyPower

Overall LGTM. Just a few comments about the naming.

You use Util for RelativeTimeUtil and ByteUnitUtil and Utils for ValueValidationUtils. Should we use a consistent naming style?

pulsar-cli-utils/src/main/java/org/apache/pulsar/cli/converters/RelativeTimeUtil.java

…odule

JooHyukKim · 2023-08-16T14:21:19Z

/pulsarbot rerun-failure-checks

JooHyukKim · 2023-08-17T12:41:26Z

/pulsarbot rerun-failure-checks

tisonkun

LGTM. Thanks for your contribution!

tisonkun · 2023-08-23T11:24:25Z

@JooHyukKim Maybe you can open a tracking issue to list out the subtasks and finish them one by one. In this way, we can share the same page what is remaining and maybe offer some helps.

JooHyukKim · 2023-08-23T13:43:38Z

@tisonkun Oh yes sounds like a good idea. Will do 👍🏻

…20782) Co-authored-by: ran <[email protected]> (cherry picked from commit 4f9b199)

JooHyukKim added 9 commits July 11, 2023 21:53

Add new module

5f4b14f

Fix maven

4945b0e

Fix compile error wrt validation utils

ceb3fc4

Add apache commons dep

45b2120

Add test for ValueValidationUtilsTest

963a7e3

Refactor test methods

cbdda80

Fix tests and compile errors

346a361

Fix checkstyle

25a93ab

Fix build failure wrt findBugsFIlter

b4e2971

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 11, 2023

tisonkun added the ready-to-test label Jul 12, 2023

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

91e5caf

…odule

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

848fe00

…odule

JooHyukKim mentioned this pull request Jul 16, 2023

[feat][cli] Add command line option for configuring the memory limit #20663

Merged

15 tasks

tisonkun self-requested a review July 17, 2023 06:13

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

e28e104

…odule

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

59a6bed

…odule

BewareMyPower assigned JooHyukKim Jul 21, 2023

BewareMyPower requested review from gaoran10, BewareMyPower and mattisonchao July 21, 2023 05:26

BewareMyPower reviewed Jul 21, 2023

View reviewed changes

pulsar-cli-utils/pom.xml Outdated Show resolved Hide resolved

JooHyukKim requested a review from gaoran10 July 31, 2023 00:11

Technoboy- added this to the 3.2.0 milestone Jul 31, 2023

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

238b590

…odule

gaoran10 reviewed Aug 4, 2023

View reviewed changes

pulsar-cli-utils/src/main/java/org/apache/pulsar/cli/converters/RelativeTimeUtil.java Outdated Show resolved Hide resolved

gaoran10 approved these changes Aug 4, 2023

View reviewed changes

Remove whitespace

f0f9f9c

Co-authored-by: ran <[email protected]>

BewareMyPower reviewed Aug 4, 2023

View reviewed changes

pulsar-cli-utils/src/main/java/org/apache/pulsar/cli/converters/RelativeTimeUtil.java Show resolved Hide resolved

Add more test covreatge and reformat

a9eb0d5

JooHyukKim requested a review from BewareMyPower August 5, 2023 00:03

JooHyukKim added 3 commits August 12, 2023 21:06

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

44dc72f

…odule

Remove unncessary comment

e0ad3bf

Merge remote-tracking branch 'upstream/master' into pip280-separate-m…

b4b0ba7

…odule

BewareMyPower approved these changes Aug 16, 2023

View reviewed changes

tisonkun approved these changes Aug 23, 2023

View reviewed changes

tisonkun merged commit 4f9b199 into apache:master Aug 23, 2023
44 of 45 checks passed

JooHyukKim deleted the pip280-separate-module branch August 23, 2023 12:16

This was referenced Aug 23, 2023

[refactor][cli][PIP-280] Tracker : Refactor byte, time parsing logic using new pulsar-cli-utils module #21056

Closed

[refactor][cli][PIP-280] Add missing pulsar-cli-utils module registration in Maven core-modules profile #21062

Merged

lhotari mentioned this pull request Feb 28, 2024

[Bug] pulsar-admin set-retention reports an invalid retention policy when size != -1 #22138

Closed

2 tasks

thetumbled pushed a commit to thetumbled/pulsar that referenced this pull request Feb 28, 2024

[refactor][cli] [PIP-280] Create new pulsar-cli-utils module (apache#…

e558bb3

…20782) Co-authored-by: ran <[email protected]> (cherry picked from commit 4f9b199)

This was referenced Feb 28, 2024

[cherry-pick] [branch-3.0] Fix getting last message ID when there are ongoing transactions #22130

Closed

[fix] [branch-3.0] fix compile error in branch 3.0 #22143

Closed

thetumbled pushed a commit to thetumbled/pulsar that referenced this pull request Feb 28, 2024

[refactor][cli] [PIP-280] Create new pulsar-cli-utils module (apache#…

7639c66

…20782) Co-authored-by: ran <[email protected]> (cherry picked from commit 4f9b199)

thetumbled pushed a commit to thetumbled/pulsar that referenced this pull request Feb 28, 2024

[refactor][cli] [PIP-280] Create new pulsar-cli-utils module (apache#…

265775f

…20782) Co-authored-by: ran <[email protected]> (cherry picked from commit 4f9b199)

heesung-sn mentioned this pull request Feb 28, 2024

[improve][admin] Expose the offload threshold in seconds to the admin #22101

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor][cli][PIP-280] Create new `pulsar-cli-utils` module #20782

[refactor][cli][PIP-280] Create new `pulsar-cli-utils` module #20782

JooHyukKim commented Jul 11, 2023 •

edited

Loading

codecov-commenter commented Jul 12, 2023 •

edited

Loading

JooHyukKim commented Jul 12, 2023

JooHyukKim commented Jul 12, 2023 •

edited

Loading

JooHyukKim commented Jul 13, 2023 •

edited

Loading

JooHyukKim commented Jul 16, 2023

JooHyukKim commented Jul 19, 2023

JooHyukKim commented Jul 19, 2023 •

edited

Loading

tisonkun commented Jul 20, 2023

gaoran10 left a comment

BewareMyPower left a comment

JooHyukKim commented Aug 16, 2023

JooHyukKim commented Aug 17, 2023

tisonkun left a comment

tisonkun commented Aug 23, 2023

JooHyukKim commented Aug 23, 2023

[refactor][cli][PIP-280] Create new pulsar-cli-utils module #20782

[refactor][cli][PIP-280] Create new pulsar-cli-utils module #20782

Conversation

JooHyukKim commented Jul 11, 2023 • edited Loading

Motivation

Modifications

Note

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

codecov-commenter commented Jul 12, 2023 • edited Loading

Codecov Report

JooHyukKim commented Jul 12, 2023

JooHyukKim commented Jul 12, 2023 • edited Loading

JooHyukKim commented Jul 13, 2023 • edited Loading

JooHyukKim commented Jul 16, 2023

JooHyukKim commented Jul 19, 2023

JooHyukKim commented Jul 19, 2023 • edited Loading

tisonkun commented Jul 20, 2023

gaoran10 left a comment

Choose a reason for hiding this comment

BewareMyPower left a comment

Choose a reason for hiding this comment

JooHyukKim commented Aug 16, 2023

JooHyukKim commented Aug 17, 2023

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun commented Aug 23, 2023

JooHyukKim commented Aug 23, 2023

[refactor][cli][PIP-280] Create new `pulsar-cli-utils` module #20782

[refactor][cli][PIP-280] Create new `pulsar-cli-utils` module #20782

JooHyukKim commented Jul 11, 2023 •

edited

Loading

codecov-commenter commented Jul 12, 2023 •

edited

Loading

JooHyukKim commented Jul 12, 2023 •

edited

Loading

JooHyukKim commented Jul 13, 2023 •

edited

Loading

JooHyukKim commented Jul 19, 2023 •

edited

Loading