You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/03/10 16:32:50 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2562: Update FateCommand to use CommandLine options
dlmarion opened a new pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562
Closes #2560
--
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
[GitHub] [accumulo] dlmarion commented on pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#issuecomment-1064261066
Output looks like:
```
usage: fate [-?] [-cancel <txid> | -delete <txid> | -dump <txid> | -fail <txid> | -list <txid> | -print <txid>] [-np] [-s <arg>] [-t <arg>]
description: manage FATE transactions
-?,--help display this help
-cancel,--cancel-submitted <txid> cancel new or submitted FaTE transactions
-delete,--delete <txid> delete locks associated with FaTE transactions (requires Manager to be down)
-dump,--dump <txid> dump FaTE transaction information details
-fail,--fail <txid> Transition FaTE transaction status to FAILED_IN_PROGRESS (requires Manager to be down)
-list,--list <txid> print FaTE transaction information
-np,--no-pagination disables pagination of output
-print,--print <txid> print FaTE transaction information
-s,--secret <arg> specify the instance secret to use
-t,--status-type <arg> filter 'print' on the transaction status type(s) {NEW, SUBMITTED, IN_PROGRESS, FAILED_IN_PROGRESS, FAILED, SUCCESSFUL}
```
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r824163409
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -108,9 +109,8 @@ public JsonElement serialize(byte[] link, Type type, JsonSerializationContext co
}
}
- private Option secretOption;
- private Option statusOption;
- private Option disablePaginationOpt;
+ private Option cancel, delete, dump, fail, list, print, secretOption, statusOption,
+ disablePaginationOpt;
Review comment:
For what it's worth, this is the recommendation from the [Code Conventions For the Java Programming Language](https://www.oracle.com/java/technologies/javase/codeconventions-declarations.html#2992)
--
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
[GitHub] [accumulo] dlmarion commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r823988291
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -243,7 +248,7 @@ private boolean cancelSubmittedTxs(final Shell shellState, String[] args)
throws AccumuloException, AccumuloSecurityException {
ClientContext context = shellState.getContext();
for (int i = 1; i < args.length; i++) {
- Long txid = Long.parseLong(args[i]);
+ Long txid = Long.parseLong(args[i], 16);
Review comment:
> Also, was this a bug before that the radix wasn't 16?
Yes, Mike found it in testing.
--
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
[GitHub] [accumulo] dlmarion commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r824033648
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -243,7 +248,7 @@ private boolean cancelSubmittedTxs(final Shell shellState, String[] args)
throws AccumuloException, AccumuloSecurityException {
ClientContext context = shellState.getContext();
for (int i = 1; i < args.length; i++) {
- Long txid = Long.parseLong(args[i]);
+ Long txid = Long.parseLong(args[i], 16);
Review comment:
Addressed in c27679a
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -108,9 +109,8 @@ public JsonElement serialize(byte[] link, Type type, JsonSerializationContext co
}
}
- private Option secretOption;
- private Option statusOption;
- private Option disablePaginationOpt;
+ private Option cancel, delete, dump, fail, list, print, secretOption, statusOption,
+ disablePaginationOpt;
Review comment:
Addressed in c27679a
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r823959785
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -243,7 +248,7 @@ private boolean cancelSubmittedTxs(final Shell shellState, String[] args)
throws AccumuloException, AccumuloSecurityException {
ClientContext context = shellState.getContext();
for (int i = 1; i < args.length; i++) {
- Long txid = Long.parseLong(args[i]);
+ Long txid = Long.parseLong(args[i], 16);
Review comment:
Also, was this a bug before that the radix wasn't 16?
--
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
[GitHub] [accumulo] dlmarion merged pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r824158064
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -108,9 +109,8 @@ public JsonElement serialize(byte[] link, Type type, JsonSerializationContext co
}
}
- private Option secretOption;
- private Option statusOption;
- private Option disablePaginationOpt;
+ private Option cancel, delete, dump, fail, list, print, secretOption, statusOption,
+ disablePaginationOpt;
Review comment:
I would love to enforce it with our formatter style, but I don't think it's 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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r823950708
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -108,9 +109,8 @@ public JsonElement serialize(byte[] link, Type type, JsonSerializationContext co
}
}
- private Option secretOption;
- private Option statusOption;
- private Option disablePaginationOpt;
+ private Option cancel, delete, dump, fail, list, print, secretOption, statusOption,
+ disablePaginationOpt;
Review comment:
nit. This makes diffs much easier to understand when code changes. I would strongly prefer one line per variable.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -243,7 +248,7 @@ private boolean cancelSubmittedTxs(final Shell shellState, String[] args)
throws AccumuloException, AccumuloSecurityException {
ClientContext context = shellState.getContext();
for (int i = 1; i < args.length; i++) {
- Long txid = Long.parseLong(args[i]);
+ Long txid = Long.parseLong(args[i], 16);
Review comment:
Not sure why this is being auto-boxed here. `ManagerClient.cancelFateOperation(context, txid)` should also take a `long` not a `Long`, because by the time it gets to `FateService.Client.cancelFateOperation(...)` it is a `long`, not a `Long`. Using `Long` just unnecessarily auto-boxes and auto-unboxes.
```suggestion
long txid = Long.parseLong(args[i], 16);
```
--
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
[GitHub] [accumulo] dlmarion commented on a change in pull request #2562: Update FateCommand to use CommandLine options
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2562:
URL: https://github.com/apache/accumulo/pull/2562#discussion_r823989244
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -108,9 +109,8 @@ public JsonElement serialize(byte[] link, Type type, JsonSerializationContext co
}
}
- private Option secretOption;
- private Option statusOption;
- private Option disablePaginationOpt;
+ private Option cancel, delete, dump, fail, list, print, secretOption, statusOption,
+ disablePaginationOpt;
Review comment:
I think GH does a pretty good job of showing you the changes in one line, but I'll change it for you. I wonder if this is something we can enforce at build time.
--
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