[#770] feat(cli): Introduce apache.commons.cli basic framework #833

slfan1989 · 2023-04-20T12:42:23Z

What changes were proposed in this pull request?

We use apache.commons.cli to add commands to Uniffle. I created a command called uniffle, the following is the case of using the command.

Case1: Use the uniffle command directly.

uniffle / uniffle --help

Usage: uniffle [OPTIONS] SUBCOMMAND [SUBCOMMAND OPTIONS]
 or    uniffle [OPTIONS] CLASSNAME [CLASSNAME OPTIONS]
  where CLASSNAME is a user-provided Java class

  OPTIONS is none or any of:

--daemon (start|status|stop)   operate on a daemon

  SUBCOMMAND is one of:


    Admin Commands:

admin-cli    prints uniffle-admin args information

    Client Commands:

client-cli   prints uniffle-cli args information

SUBCOMMAND may print help when invoked w/o parameters or with -h.

Case2: use a command that does not create.

uniffle testadmin

ERROR: testadmin is not COMMAND nor fully qualified CLASSNAME.
DEBUG: UNIFFLE_SUBCMD_USAGE_TYPES accepted client
DEBUG: UNIFFLE_SUBCMD_USAGE_TYPES accepted admin
Usage: uniffle [OPTIONS] SUBCOMMAND [SUBCOMMAND OPTIONS]
 or    uniffle [OPTIONS] CLASSNAME [CLASSNAME OPTIONS]
  where CLASSNAME is a user-provided Java class

  OPTIONS is none or any of:

--daemon (start|status|stop)   operate on a daemon

  SUBCOMMAND is one of:


    Admin Commands:

admin-cli    prints uniffle-admin args information

    Client Commands:

client-cli   prints uniffle-cli args information

SUBCOMMAND may print help when invoked w/o parameters or with -h.

Case3: Use the help command

uniffle client-cli --help

Usage:
   Optional
     -a,--admin <arg>   This is an admin command that will print args.
     -c,--cli <arg>     This is an client cli command that will print args.
     -h,--help          Help for the Uniffle CLI.

Case4: Run the example-cli command

uniffle client-cli --cli test-cli

uniffle-client-cli : test-cli

Why are the changes needed?

We use apache.commons.cli to add commands to Uniffle.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test verification.
Execute commands directly.

slfan1989 · 2023-04-20T12:44:21Z

@smallzhongfeng @jerqi Can you help to review this pr? Thank you very much!

codecov-commenter · 2023-04-21T07:14:11Z

Codecov Report

Merging #833 (b5406c1) into master (47c4966) will increase coverage by 2.22%.
The diff coverage is 54.16%.

@@             Coverage Diff              @@
##             master     #833      +/-   ##
============================================
+ Coverage     57.09%   59.32%   +2.22%     
- Complexity     2148     2165      +17     
============================================
  Files           321      305      -16     
  Lines         15655    13388    -2267     
  Branches       1243     1249       +6     
============================================
- Hits           8939     7943     -996     
+ Misses         6212     5009    -1203     
+ Partials        504      436      -68

Impacted Files	Coverage Δ
...va/org/apache/uniffle/UniffleCliArgsException.java	`0.00% <0.00%> (ø)`
.../org/apache/uniffle/AbstractCustomCommandLine.java	`54.83% <54.83%> (ø)`
...c/main/java/org/apache/uniffle/cli/UniffleCLI.java	`59.45% <59.45%> (ø)`

... and 25 files with indirect coverage changes

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

bin/uniffle-function.sh

bin/utils.sh

cli/src/test/java/org/apache/uniffle/cli/ExampleCLITest.java

pom.xml

jerqi · 2023-04-21T13:42:39Z

bin/stop-coordinator.sh

@@ -21,6 +21,7 @@ set -o pipefail
 set -o nounset   # exit the script if you try to use an uninitialised variable
 set -o errexit   # exit the script if any statement returns a non-true return value

+UNIFFLE_SHELL_SCRIPT_DEBUG=true


Could we put UNIFFLE_SHELL_SCRIPT_DEBUG=true into utils.sh and load_rss_env?

I will modify the code.

jerqi · 2023-04-21T13:43:53Z

bin/uniffle-function.sh

+function uniffle_generic_java_subcmd_handler
+{
+  uniffle_java_exec "${UNIFFLE_SUBCMD}" "${UNIFFLE_CLASSNAME}" "${UNIFFLE_SUBCMD_ARGS[@]}"
+}


Add a new line. There is a warning.

Thanks for helping to review the code, I will fix it.

jerqi · 2023-04-21T13:45:18Z

cc @Kwafoor

jerqi · 2023-04-27T03:24:28Z

cli/src/main/java/org/apache/uniffle/cli/ExampleCLI.java

+import org.apache.uniffle.UniffleCliArgsException;
+
+
+public class ExampleCLI extends AbstractCustomCommandLine {


Could we have a better name?

What about UniffleTestCLI?

Why do we need an ExampleCli? Coulw we have a UniffleCli directly?

Why do we need an ExampleCli? Coulw we have a UniffleCli directly?

Thank you for your suggestion! I will modify the class name, and I will continue to complete subTask in this class, #837, #846.

My idea is to provide a sample class for testing, so it is called exampleCLI, but it is called uniffleCLI, which is a better choice.

jerqi · 2023-04-27T03:24:52Z

bin/uniffle

+set +u
+uniffle_cmd_case "${UNIFFLE_SUBCMD}" "${UNIFFLE_SUBCMD_ARGS[@]}"
+uniffle_generic_java_subcmd_handler
+set -u


There should be a new line.

I will fix it.

jerqi · 2023-04-27T03:25:12Z

cli/pom.xml

+            <artifactId>commons-cli</artifactId>
+        </dependency>
+    </dependencies>
+</project>


There should be a new line.

I will fix it.

jerqi · 2023-04-27T06:20:39Z

Refer to Yarn, should we have ClientCli and AdminCli? Both of them extend the UniffeCli?

slfan1989 · 2023-04-27T07:07:42Z

Refer to Yarn, should we have ClientCli and AdminCli? Both of them extend the UniffeCli?

@jerqi

Thank you for your question! In the case of YARN, there are clear distinctions between client and admin operations, so it makes sense to separate them using different CLIs. This can help improve usability of the tool.

In general, I believe that Uniffle should differentiate between two sets of commands for clients and admins.

At present, there are only a few known commands for Uniffle. I suggest that we could develop them within UniffleCLI and use options to differentiate between them. If necessary, we can always consider splitting them into separate client and admin CLIs later on.

Do you think this approach would be feasible?

jerqi · 2023-04-27T07:09:04Z

Refer to Yarn, should we have ClientCli and AdminCli? Both of them extend the UniffeCli?

Thank you for your question! In the case of YARN, there are clear distinctions between client and admin operations, so it makes sense to separate them using different CLIs. This can help improve usability of the tool.

In general, I believe that Uniffle should differentiate between two sets of commands for clients and admins.

At present, there are only a few known commands for Uniffle. I suggest that we could develop them within UniffleCLI and use options to differentiate between them. If necessary, we can always consider splitting them into separate client and admin CLIs later on.

Do you think this approach would be feasible?

I'm ok.

smallzhongfeng

LGTM. also cc @jerqi After merging this PR, we can start developing some other commands.

jerqi

LGTM

jerqi · 2023-04-27T07:40:56Z

What changes were proposed in this pull request?

We use apache.commons.cli to add commands to Uniffle. I created a command called uniffle, the following is the case of using the command.

Case1: Use the uniffle command directly.

uniffle / uniffle --help
Usage: uniffle [OPTIONS] SUBCOMMAND [SUBCOMMAND OPTIONS]
 or    uniffle [OPTIONS] CLASSNAME [CLASSNAME OPTIONS]
  where CLASSNAME is a user-provided Java class

  OPTIONS is none or any of:

--daemon (start|status|stop)   operate on a daemon

  SUBCOMMAND is one of:

    Admin Commands:

example-admin   prints example-admin args information

    Client Commands:

example-cli     prints example-cli args information

SUBCOMMAND may print help when invoked w/o parameters or with -h.
Case2: use a command that does not create.

uniffle testadmin.
ERROR: testadmin is not COMMAND nor fully qualified CLASSNAME.
DEBUG: UNIFFLE_SUBCMD_USAGE_TYPES accepted client
DEBUG: UNIFFLE_SUBCMD_USAGE_TYPES accepted admin
Usage: uniffle [OPTIONS] SUBCOMMAND [SUBCOMMAND OPTIONS]
 or    uniffle [OPTIONS] CLASSNAME [CLASSNAME OPTIONS]
  where CLASSNAME is a user-provided Java class

  OPTIONS is none or any of:

--daemon (start|status|stop)   operate on a daemon

  SUBCOMMAND is one of:

    Admin Commands:

example-admin   prints example-admin args information

    Client Commands:

example-cli     prints example-cli args information

SUBCOMMAND may print help when invoked w/o parameters or with -h.
Case3: Use the help command

uniffle example-cli --help
Usage:
   Optional
     -a,--admin <arg>   This is an example admin command that will print args.
     -c,--cli <arg>     This is an example cli command that will print args.
     -h,--help          Help for the Uniffle Example-CLI.
Case4: Run the example-cli command

uniffle example-cli --cli test-cli
example-cli : test-cli
Why are the changes needed?

We use apache.commons.cli to add commands to Uniffle.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test verification.

Execute commands directly.

Do we need modify the description? Is there some update?

jerqi · 2023-04-27T08:54:15Z

@kaijchen Do you have another suggestion? You have more experience about Ratis and Ozone client. If you have another suggestion, you can raise some follow-up prs.
I prefer merging this pr first when it passed CI. It will be convenient for us.Many issues will rely on this pr.

jerqi

Merged. Thanks all

slfan1989 · 2023-04-27T11:19:28Z

@jerqi @smallzhongfeng Thank you very much for reviewing the code and providing suggestions for modification!

jerqi · 2023-04-27T11:25:46Z

Do you have Wechat acount? Have you joined Uniffle Wechat Group? If you want to join our group, you can scan

slfan1989 marked this pull request as draft April 20, 2023 12:42

jerqi requested a review from smallzhongfeng April 21, 2023 01:53

slfan1989 marked this pull request as ready for review April 21, 2023 10:40

smallzhongfeng reviewed Apr 21, 2023

View reviewed changes

bin/uniffle-function.sh Show resolved Hide resolved

bin/uniffle-function.sh Outdated Show resolved Hide resolved

bin/utils.sh Show resolved Hide resolved

cli/src/test/java/org/apache/uniffle/cli/ExampleCLITest.java Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

jerqi reviewed Apr 21, 2023

View reviewed changes

[apache#770] Introduce apache.commons.cli basic framework.

9aaf2c2

slfan1989 force-pushed the cli_basic_framework branch from fc3176c to 9aaf2c2 Compare April 26, 2023 12:30

slfan1989 added 2 commits April 26, 2023 20:32

[apache#770] Fix CheckStyle.

33dcd65

[apache#770] Fix CheckStyle.

6aedd34

jerqi reviewed Apr 27, 2023

View reviewed changes

slfan1989 added 4 commits April 27, 2023 11:25

[apache#770] Fix CheckStyle.

a3cc8ca

[apache#770] Fix CheckStyle.

7a919b8

[apache#770] Fix CheckStyle.

ada0831

[apache#770] Fix CheckStyle.

b5406c1

smallzhongfeng previously approved these changes Apr 27, 2023

View reviewed changes

jerqi previously approved these changes Apr 27, 2023

View reviewed changes

[apache#770] Fix CheckStyle.

2fe59ff

slfan1989 dismissed stale reviews from jerqi and smallzhongfeng via 2fe59ff April 27, 2023 07:53

slfan1989 added 2 commits April 27, 2023 16:27

[apache#770] Fix CheckStyle.

11ff006

[apache#770] Fix CheckStyle.

37fc10c

jerqi changed the title ~~[#770] Introduce apache.commons.cli basic framework~~ [#770] feat(cli): Introduce apache.commons.cli basic framework Apr 27, 2023

jerqi requested a review from kaijchen April 27, 2023 08:51

[apache#770] Fix CheckStyle.

d883389

jerqi approved these changes Apr 27, 2023

View reviewed changes

jerqi merged commit a3ca258 into apache:master Apr 27, 2023

smallzhongfeng changed the title ~~[#770] feat(cli): Introduce apache.commons.cli basic framework~~ [#758] feat(cli): Introduce apache.commons.cli basic framework May 6, 2023

smallzhongfeng changed the title ~~[#758] feat(cli): Introduce apache.commons.cli basic framework~~ [#770] feat(cli): Introduce apache.commons.cli basic framework May 6, 2023

smallzhongfeng mentioned this pull request May 6, 2023

[Subtask] Introduce apache.commons.cli basic framework #770

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#770] feat(cli): Introduce apache.commons.cli basic framework #833

[#770] feat(cli): Introduce apache.commons.cli basic framework #833

slfan1989 commented Apr 20, 2023 •

edited

Loading

slfan1989 commented Apr 20, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

jerqi Apr 21, 2023

slfan1989 Apr 27, 2023

jerqi Apr 21, 2023

slfan1989 Apr 27, 2023

jerqi commented Apr 21, 2023

jerqi Apr 27, 2023

slfan1989 Apr 27, 2023

jerqi Apr 27, 2023 •

edited

Loading

slfan1989 Apr 27, 2023

jerqi Apr 27, 2023

slfan1989 Apr 27, 2023

jerqi Apr 27, 2023

slfan1989 Apr 27, 2023

jerqi commented Apr 27, 2023 •

edited

Loading

slfan1989 commented Apr 27, 2023 •

edited

Loading

jerqi commented Apr 27, 2023

smallzhongfeng left a comment

jerqi left a comment

jerqi commented Apr 27, 2023 •

edited

Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerqi commented Apr 27, 2023

jerqi left a comment

slfan1989 commented Apr 27, 2023

jerqi commented Apr 27, 2023

		import org.apache.uniffle.UniffleCliArgsException;


		public class ExampleCLI extends AbstractCustomCommandLine {

[#770] feat(cli): Introduce apache.commons.cli basic framework #833

[#770] feat(cli): Introduce apache.commons.cli basic framework #833

Conversation

slfan1989 commented Apr 20, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

slfan1989 commented Apr 20, 2023 • edited Loading

codecov-commenter commented Apr 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Apr 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Apr 27, 2023 • edited Loading

slfan1989 commented Apr 27, 2023 • edited Loading

jerqi commented Apr 27, 2023

smallzhongfeng left a comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

jerqi commented Apr 27, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerqi commented Apr 27, 2023

jerqi left a comment

Choose a reason for hiding this comment

slfan1989 commented Apr 27, 2023

jerqi commented Apr 27, 2023

slfan1989 commented Apr 20, 2023 •

edited

Loading

slfan1989 commented Apr 20, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

jerqi Apr 27, 2023 •

edited

Loading

jerqi commented Apr 27, 2023 •

edited

Loading

slfan1989 commented Apr 27, 2023 •

edited

Loading

jerqi commented Apr 27, 2023 •

edited

Loading