[refactor][cli][PIP-280] Refactor `pulsar-client-tools` module by JooHyukKim · Pull Request #20764 · apache/pulsar

JooHyukKim · 2023-07-09T13:16:24Z

PIP: #20691

Motivation

In the current Pulsar codebase, the logic to parse CLI arguments for measurement units like time and bytes is scattered across various CLI classes. Each value read has its distinct parsing implementation, leading to a lack of code reuse.

Modifications

Implement validators and converters to replace manual parsing logic in Cmd classes
Apply refactor to...
- CmdNamespaces
- CmdPersistetTopics
- CmdTopicPolicies
- CmdTopics
- TimeUnitTo

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

…mespaces" This reverts commit 1ea46bb, reversing changes made to 90e51ed.

This reverts commit 26d3348.

JooHyukKim · 2023-09-01T13:24:23Z

@tisonkun May I ask for a review? 🙏🏼

tisonkun · 2023-09-15T13:17:46Z

Thanks for your contribution! I'll give a review this weekend.

tisonkun

Thanks for your contribution @JooHyukKim!

This patch is in a good direction. Comments inline. And please check the CI failure.

tisonkun · 2023-09-18T09:23:47Z

+            final int retentionTimeInMin = retentionTimeInSec !=  -1
+                    ? (int) TimeUnit.SECONDS.toMinutes(retentionTimeInSec)
+                    : retentionTimeInSec.intValue();
+            final int retentionSizeInMB = sizeLimit != -1


Perhaps we can later move this size conversion to ByteUnitUtil also.

True true 👍🏼, added to #21056

tisonkun · 2023-09-18T09:24:35Z

            if (backlogQuotaType == BacklogQuota.BacklogQuotaType.destination_storage) {
                // set quota by storage size
-                if (limitStr == null) {
+                if (limit == null) {


We may use a Nonnull ValueValidator here?

Hmmm, I did not touch limit and limitTimeInSec because they both have meaningful exception message such as...

throw new ParameterException("Quota type of 'destination_storage' needs a size limit");

... this, so may not fit, and plus Validators and Converters are both passed static via annotation, so... kinda tricky.

Thanks for your explanation! Let's keep it AS IS then.

tisonkun

LGTM. We should try to fix the CI issue.

JooHyukKim · 2023-09-19T22:22:32Z

Thanks for the review! @tisonkun

I suspect the OWASP dependency check workflow might not run for commits/PRs from maintainers. This might slow down fixes, but I haven't yet succeeded in verifying. 🧐

JooHyukKim · 2023-09-22T11:18:49Z

/pulsarbot rerun-failure-checks

tisonkun · 2023-09-25T09:08:31Z

@JooHyukKim Please read the CI output before retry, it gives:

pulsar-cli-utils-3.2.0-SNAPSHOT.jar unaccounted for in LICENSE

So you may need to update some LICENSE file. Please check the CI output for details.

https://github.com/apache/pulsar/actions/runs/6283293139/job/17063761526?pr=20764

JooHyukKim · 2023-09-30T13:14:18Z

Converted to draft PR as we have couple of failures to fix.
Anyone is welcome to pick up and go on.

tisonkun · 2023-10-10T14:37:28Z

Rebase on #20663. Let's test once more.

Signed-off-by: tison <wander4096@gmail.com>

codecov-commenter · 2023-10-11T03:45:09Z

Codecov Report

Attention: Patch coverage is 72.22222% with 15 lines in your changes missing coverage. Please review.

Project coverage is 73.27%. Comparing base (c35524d) to head (cf43812).
Report is 1566 commits behind head on master.

Files with missing lines	Patch %	Lines
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java	61.11%	6 Missing and 1 partial ⚠️
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java	76.19%	2 Missing and 3 partials ⚠️
.../org/apache/pulsar/admin/cli/CmdTopicPolicies.java	85.71%	0 Missing and 2 partials ⚠️
...g/apache/pulsar/admin/cli/CmdPersistentTopics.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20764      +/-   ##
============================================
+ Coverage     73.25%   73.27%   +0.02%     
+ Complexity    32517    32510       -7     
============================================
  Files          1888     1888              
  Lines        140246   140067     -179     
  Branches      15443    15408      -35     
============================================
- Hits         102736   102633     -103     
+ Misses        29419    29373      -46     
+ Partials       8091     8061      -30

Flag	Coverage Δ
inttests	`24.30% <0.00%> (+0.06%)`	⬆️
systests	`24.71% <0.00%> (-0.03%)`	⬇️
unittests	`72.58% <72.22%> (+0.04%)`	⬆️

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

Files with missing lines	Coverage Δ
...n/java/org/apache/pulsar/admin/cli/CliCommand.java	`59.15% <ø> (+1.20%)`	⬆️
...g/apache/pulsar/admin/cli/CmdPersistentTopics.java	`65.02% <0.00%> (+0.24%)`	⬆️
.../org/apache/pulsar/admin/cli/CmdTopicPolicies.java	`90.67% <85.71%> (+2.02%)`	⬆️
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java	`78.39% <76.19%> (+1.96%)`	⬆️
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java	`81.17% <61.11%> (+2.22%)`	⬆️

... and 62 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tisonkun · 2023-10-16T11:54:00Z

Thanks for your contributon! The first migration step is merged now :D

…e#20764) Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: tison <wander4096@gmail.com>

JooHyukKim · 2023-10-17T15:52:59Z

@tisonkun Thank you also for considerate time investment on this one! I will try to pick up where I left off 👍🏼.

JooHyukKim added 21 commits July 1, 2023 17:58

Implement for CmdNamespaces

4924135

Fix changes

90e51ed

Merge remote-tracking branch 'upstream/master' into pip280-namespaces

1ea46bb

Fix coerced validation

4a2e2f7

Update

94bb470

Revert "Merge remote-tracking branch 'upstream/master' into pip280-na…

9914aee

…mespaces" This reverts commit 1ea46bb, reversing changes made to 90e51ed.

Fix name and checkstyle

82a17c5

Clean up test

7e2ae5b

Fix tests and validations

72f1bfe

Temp change

26d3348

Revert revert merge upstream

d528acd

Revert "Temp change"

e9f3630

This reverts commit 26d3348.

Fixing changes via PulsarAdminToolConverterTest

df3279f

Fix human error wrt long/integer converter

9d3a9c1

Update PulsarAdminToolConverterTest.java

8aee04f

Refactor CmdTopics

c1f4388

Refactor CmdTopicPolicies

9ddb051

Clean up changes

a354c33

.

78627ce

Fix CmdTopics

315a463

Merge remote-tracking branch 'upstream/master' into PIP280-ALL

7a77e08

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

Add new module

5f4b14f

JooHyukKim marked this pull request as draft July 11, 2023 12:57

JooHyukKim added 6 commits July 11, 2023 22:20

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

Merge remote-tracking branch 'upstream/master' into PIP280-CmdTopics

5dd1c77

JooHyukKim changed the title ~~[refactor][cli][PIP-280] Refactor CLI parameters in pulsar-client-tools module~~ [refactor][cli][PIP-280] Refactor pulsar-client-tools module Sep 4, 2023

JooHyukKim and others added 2 commits September 4, 2023 20:39

Merge remote-tracking branch 'upstream/master' into PIP280-CmdTopics

3581d55

Merge branch 'master' into PIP280-CmdTopics

aeeb9f7

tisonkun requested review from BewareMyPower, Technoboy- and gaoran10 September 15, 2023 13:18

tisonkun reviewed Sep 18, 2023

View reviewed changes

Update CmdTopics.java

fbcf8de

JooHyukKim requested a review from tisonkun September 18, 2023 13:39

tisonkun approved these changes Sep 19, 2023

View reviewed changes

Merge branch 'master' into PIP280-CmdTopics

edf7b90

JooHyukKim marked this pull request as draft September 27, 2023 15:14

Merge branch 'master' into PIP280-CmdTopics

0ad6cda

tisonkun marked this pull request as ready for review October 10, 2023 15:34

retrigger CI

cf43812

Signed-off-by: tison <wander4096@gmail.com>

tisonkun requested review from lhotari and nicoloboschi October 11, 2023 03:41

tisonkun merged commit a7acdf6 into apache:master Oct 16, 2023

vraulji567 pushed a commit to vraulji567/pulsar that referenced this pull request Oct 16, 2023

[refactor][cli][PIP-280] Refactor pulsar-client-tools module (apach…

3d5bcbc

…e#20764) Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: tison <wander4096@gmail.com>

Conversation

JooHyukKim commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

JooHyukKim commented Sep 1, 2023

Uh oh!

tisonkun commented Sep 15, 2023

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

tisonkun Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

tisonkun Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

JooHyukKim commented Sep 19, 2023

Uh oh!

JooHyukKim commented Sep 22, 2023

Uh oh!

tisonkun commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JooHyukKim commented Sep 30, 2023

Uh oh!

tisonkun commented Oct 10, 2023

Uh oh!

codecov-commenter commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tisonkun commented Oct 16, 2023

Uh oh!

JooHyukKim commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JooHyukKim commented Jul 9, 2023 •

edited

Loading

tisonkun commented Sep 25, 2023 •

edited

Loading

codecov-commenter commented Oct 11, 2023 •

edited

Loading