Use the natively supported version and usage by picocli by jonas-grgt · Pull Request #10672 · apache/pinot

jonas-grgt · 2023-04-23T18:04:08Z

Instructions:

The PR has to be tagged with at least one of the following labels (*):
1. feature
2. bugfix
Remove these instructions before publishing the PR.

(*) Other labels to consider:

testing
refactor
cleanup

The general idea was to make use of the natively supported mixinStandardHelpOptions of picocli.

I took the liberty to add some tests for the PinotAdministrator.

Amy feedback more then welcome I'm happy to adjust the code in order to get it merged in and improve pinot!

walterddr

thank you for the PR, can you clean up the linter issue and submit again.

jonas-grgt · 2023-05-01T19:35:50Z

thank you for the PR, can you clean up the linter issue and submit again.

I ran checkstyle and fixed all open issues.

jonas-grgt · 2023-05-02T05:27:31Z

thank you for the PR, can you clean up the linter issue and submit again.

There seems to be more then checkstyle in place. I suppose I need to get the built green. I'll look into it.

codecov-commenter · 2023-05-02T14:34:23Z

Codecov Report

Merging #10672 (60ec1f8) into master (815cd6e) will increase coverage by 0.05%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #10672      +/-   ##
============================================
+ Coverage     64.38%   64.43%   +0.05%     
- Complexity     6441     6462      +21     
============================================
  Files          2068     2106      +38     
  Lines        111794   113512    +1718     
  Branches      16956    17262     +306     
============================================
+ Hits          71974    73137    +1163     
- Misses        34646    35122     +476     
- Partials       5174     5253      +79

Flag	Coverage Δ
unittests1	`67.86% <0.00%> (-0.04%)`	⬇️
unittests2	`13.69% <0.00%> (-0.15%)`	⬇️

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

Impacted Files	Coverage Δ
...rc/main/java/org/apache/pinot/common/Versions.java	`0.00% <0.00%> (ø)`

... and 219 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pinot-tools/src/main/java/org/apache/pinot/tools/admin/PinotAdministrator.java

jonas-grgt · 2023-05-23T19:19:46Z

@walterddr Saw an attempt was made to built the project but, failed due to incompatibility with java 8.

Think I forgot to actually push my changes 🙈

In any case it should now pass, can you try again ?

Also what is the reason for supporting JDK-8 as a minimum version ?

walterddr · 2023-05-23T21:18:22Z

Also what is the reason for supporting JDK-8 as a minimum version ?

I think it was because some users were still on JDK8. we are in the process of removing it (for a while unfortunately)
I will take another review later today. thank you for the contribution

walterddr

did a brief pass through the PR. please kindly take a look at my comments.

thank you for addressing the JDK8 issue. it is tedious for sure

walterddr · 2023-05-24T18:08:43Z

pinot-tools/src/test/java/org/apache/pinot/tools/admin/PinotAdministratorTest.java

+ */
+package org.apache.pinot.tools.admin;
+
+import java.io.StringWriter;


Suggested change

import java.io.StringWriter;

import com.google.common.collect.ImmutableMap;

import java.io.StringWriter;

walterddr · 2023-05-24T18:08:59Z

pinot-tools/src/test/java/org/apache/pinot/tools/admin/PinotAdministratorTest.java

+  static class VersionsStub extends Versions {
+    @Override
+    public Map<String, String> getComponentVersions() {
+      return new TreeMap<>(Map.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));


Suggested change

return new TreeMap<>(Map.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));

return new TreeMap<>(ImmutableMap.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));

walterddr · 2023-05-24T18:10:12Z

pinot-tools/src/test/java/org/apache/pinot/tools/admin/PinotAdministratorTest.java

+    pinotAdministrator.execute(new String[]{"AddSchema", "--help"});
+
+    // then
+    assertEquals(out.toString(), "Usage: <main class> AddSchema [-hV] [-exec] [-authToken=<_authToken>]\n"


this assertEquals is going to be hard to maintain if we add additional commands. was wondering how other project use picocli handles unit-test? do we have a util to generate these expected results?

walterddr · 2023-05-24T18:14:27Z

pinot-tools/src/main/java/org/apache/pinot/tools/perf/QueryRunner.java

-        + "  QueryRunner -mode singleThread -queryFile <queryFile> -numTimesToRunQueries 0 "
-        + "-numIntervalsToReportAndClearStatistics 5\n"
-        + "  QueryRunner -mode multiThreads -queryFile <queryFile> -numThreads 10 -reportIntervalMs 1000\n"
-        + "  QueryRunner -mode targetQPS -queryFile <queryFile> -startQPS 50\n"
-        + "  QueryRunner -mode increasingQPS -queryFile <queryFile> -startQPS 50 -deltaQPS 10 "
-        + "-numIntervalsToIncreaseQPS 20\n";


did we missed these examples in the new format?

walterddr · 2023-05-24T18:15:01Z

pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/filesystem/MoveFiles.java

+//  @Override
+//  public void getDescription() {
+//    System.out.println("mv [-o] <source-uri> <destination-uri>");
+//  }


remove these comments if no longer needed

walterddr · 2023-05-24T18:16:25Z

pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/RebalanceTableCommand.java

-  }
-
-  @Override
-  public void printExamples() {


is this description refactored anywhere in the new code? can't find it

jonas-grgt changed the title ~~Use the natively supported version and usage by picocli~~ [Draft] Use the natively supported version and usage by picocli Apr 23, 2023

jonas-grgt changed the title ~~[Draft] Use the natively supported version and usage by picocli~~ Use the natively supported version and usage by picocli Apr 24, 2023

walterddr self-requested a review April 26, 2023 15:22

Jackie-Jiang added feature bugfix labels Apr 26, 2023

walterddr reviewed Apr 28, 2023

View reviewed changes

walterddr reviewed May 2, 2023

View reviewed changes

pinot-tools/src/main/java/org/apache/pinot/tools/admin/PinotAdministrator.java Outdated Show resolved Hide resolved

jonas-grgt added 3 commits May 23, 2023 21:16

Use the natively supported version and usage by picocli

253389f

Fix checkstyle issues

ade1e52

Make code JDK 8 compatible by removing vars

60ec1f8

walterddr reviewed May 24, 2023

View reviewed changes

jonas-grgt closed this by deleting the head repository Nov 13, 2023

	import java.io.StringWriter;
	import com.google.common.collect.ImmutableMap;
	import java.io.StringWriter;

	return new TreeMap<>(Map.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));
	return new TreeMap<>(ImmutableMap.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));

Conversation

jonas-grgt commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

jonas-grgt commented May 1, 2023

Uh oh!

jonas-grgt commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

jonas-grgt commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walterddr commented May 23, 2023

Uh oh!

walterddr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr May 24, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr May 24, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr May 24, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr May 24, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr May 24, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonas-grgt commented Apr 23, 2023 •

edited

Loading

jonas-grgt commented May 2, 2023 •

edited

Loading

codecov-commenter commented May 2, 2023 •

edited

Loading

jonas-grgt commented May 23, 2023 •

edited

Loading

walterddr left a comment •

edited

Loading