You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2025/04/07 12:39:11 UTC
[PR] Added 'a' short option to Shell merge --all command [accumulo]
dlmarion opened a new pull request, #5455:
URL: https://github.com/apache/accumulo/pull/5455
Prior to #5450 the all option in the merge command used an empty
string for the short option. As part of which caused a failure because
commons-cli no longer allows an empty string as the short option
(see [1]). without a short option, which is allowed, but that appears
to have caused failures in ShellIT when the `merge --all` command
is used. This commit simply adds 'a' as the short option and removes
the Option.Builder modification.
[1] https://github.com/apache/commons-cli/commit/0ece5e45c0d5c713f1c3888281f8eb32c252d5db
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion closed pull request #5455: Added 'a' short option to Shell merge --all command
URL: https://github.com/apache/accumulo/pull/5455
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031716653
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
Man, should have thought of that....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031590825
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
If want to avoid adding the `-a` option then maybe could keep this builder code as is and change the following code which is a bit further up in the class from
```java
if (cl.hasOption(allOpt.getOpt())) {
all = true;
}
```
to
```java
if (cl.hasOption(allOpt.getLongOpt())) {
all = true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031786006
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
> I wonder if all the code should use this and if it means some of the existing code is not handling long options properly. Like will only the -f options work for the merge command and not the --force option?
Looked into this and it seems to work. When `-f` string is passed to CLI code it will use that to find the Option object and would still match if `--force` was passed in, but that only works when there is a short option. Still seems better to just pass the option object though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031728108
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
Do we really need to decide which option string to check for?
CommandLine has a `hasOption` method that takes the entire `Option` object instead of choosing between the `short` and `long` opt values.
https://commons.apache.org/proper/commons-cli/apidocs/org/apache/commons/cli/CommandLine.html#hasOption(Option)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031760538
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
The `getOptionValue` has a similar issue. If you use `getOptionValue(Option)`, then you should be good. If you use `getOptionValue(String)`, then you might be missing something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031731019
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
I personally don't see an issue with adding a `-a` short option but I understand wanting to keep the code as minimal as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031751989
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
> Do we really need to decide which option string to check for?
CommandLine has a hasOption method that takes the entire Option object instead of choosing between the short and long opt values.
https://commons.apache.org/proper/commons-cli/apidocs/org/apache/commons/cli/CommandLine.html#hasOption(Option)
I wonder if all the code should use this and if it means some of the existing code is not handling long options properly. Like will only the `-f` options work for the merge command and not the `--force` option?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#discussion_r2031746749
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -95,11 +95,9 @@ public Options getOptions() {
new Option("s", "size", true, "merge tablets to the given size over the entire table");
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given size");
- // Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
Review Comment:
I put up #5456 to just check the option and not add the '-a' short option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added 'a' short option to Shell merge --all command [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5455:
URL: https://github.com/apache/accumulo/pull/5455#issuecomment-2784472968
Closing in favor of #5456
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org