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/09/02 18:25:37 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2914: Move new FateCommand options to Admin
milleruntime opened a new pull request, #2914:
URL: https://github.com/apache/accumulo/pull/2914
* See discussion on #2780 about problems trying to complete #2215
* Move new cancel and summary options to Admin and deprecate
FateCommand.
--
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] milleruntime commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1245881953
I want to do more testing with the new commands but this PR is ready for review. I had to make some changes with the commands being moved out of the shell, mainly dropping the FateCommand unit tests and the pagination option, but it was things that didn't translate over to the Admin command.
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248394580
> > I think you are right that it will be confusing to split the command up between the shell and the admin command. I think it will be better to just deprecate the command here and then move it completely to Admin in the next version.
>
> I'm not sure I understand this statement. Are you proposing for it to be deprecated without a replacement in 2.1, and then in 3.0 to move it to the Admin command?
Yes.
> I would prefer it exist in the Admin command at the time we deprecate it (which I prefer in 2.1, so we can remove it in 3.0). I do not care, though, if the new options are available or not in the deprecated shell version.
What do you prefer exists in the Admin command? The entire fate command?
--
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] EdColeman commented on a diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r975622827
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private static final String warning =
+ "WARNING: This command is deprecated for removal. Use 'accumulo admin'\n";
Review Comment:
suggest ```
"WARNING: This command is deprecated for removal. Use 'accumulo admin' and to use new options\n";
```
--
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 pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248412478
> > > I think you are right that it will be confusing to split the command up between the shell and the admin command. I think it will be better to just deprecate the command here and then move it completely to Admin in the next version.
> >
> >
> > I'm not sure I understand this statement. Are you proposing for it to be deprecated without a replacement in 2.1, and then in 3.0 to move it to the Admin command?
>
> Yes.
I don't think we should deprecate without a replacement. The replacement should be in place, or it would be confusing to have it deprecated.
>
> > I would prefer it exist in the Admin command at the time we deprecate it (which I prefer in 2.1, so we can remove it in 3.0). I do not care, though, if the new options are available or not in the deprecated shell version.
>
> What do you prefer exists in the Admin command? The entire fate command?
Anything being deprecated/replaced.
If it is deprecated now, then moved later, then users will never be pointed in the direction of the new command... because in the deprecated code, the new command won't exist, and when the new command is created, the deprecation won't exist. They need to exist at the same time for a deprecation warning/message to be useful to users.
--
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] milleruntime commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1239804776
@EdColeman while moving the test testSummary() from FateCommandTest, I noticed that it is calling the overridden method in TestFateCommand class. Was that the intention? That class was really only created for testCommandLineOptions().
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1252448795
I duplicated the FateCommand in Admin. To be more obvious, I overrode the usage to print the deprecation warning first and printed the warning in shell. So it won't matter how the command is called, the user should get the message.
The only question now is if we should drop `FateAdmin`. This version of Accumulo also includes 2 classes for `FateAdmin`, in the master and manager packages. I think it should be dropped in this version. A previous version deprecated `FateAdmin` in favor of the shell. This version we deprecated the shell FateCommand in favor of the Admin command.
--
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] EdColeman commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1239260751
Operators are familiar with the current shell fate operations and may be using them more that expected. I'm not opposed to moving it to the admin command, but the same functionality should be preserved. It would also be nice if the shell could print a message that points to the moved command and at least a hint on how to execute it.
--
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] milleruntime commented on a diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r976468365
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private static final String warning =
+ "WARNING: This command is deprecated for removal. Use 'accumulo admin'\n";
Review Comment:
I think we accomplished that with the message printing when trying to use the command. I added you as a co-auther on the commit.
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1252698608
@EdColeman correct me if I am wrong but I think you are saying: "It would be nice if the new options (cancel and summary) were available in `FateCommand` exposing them to users".
I will look at making the new options in `FateCommand` call code in Admin.
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1249729592
More on differentiating list and print... This is due to the way that JCommander works. We can't use an ArrayList as the type because there is no way to know if the user entered `fate -list` with 0 txIds or didn't enter the command. So that's why I was thinking we need a second different type boolean for a `listAll` option. The user won't know any TxIds from the start, so we still need a `listAll` 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
[GitHub] [accumulo] dlmarion commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1249773667
`fate -list all`
`fate -list tx1 tx2 tx3`
Make `all` a reserved word effectively for the list command?
--
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 pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248357076
> I think you are right that it will be confusing to split the command up between the shell and the admin command. I think it will be better to just deprecate the command here and then move it completely to Admin in the next version.
I'm not sure I understand this statement. Are you proposing for it to be deprecated without a replacement in 2.1, and then in 3.0 to move it to the Admin command?
I would prefer it exist in the Admin command at the time we deprecate it (which I prefer in 2.1, so we can remove it in 3.0). I do not care, though, if the new options are available or not in the deprecated shell version.
--
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] EdColeman commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1252788865
I think it would be enough it if was.
```
fate -cancel 3cb69aefefe6b6d0
2022-09-20T14:18:48,053 [shell.Shell] WARN : WARNING: This command is deprecated for removal. Use 'accumulo admin'
This option in not available - use 'accumulo admin'
```
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1253661431
It is a bit strange to add new options to the Shell FateCommand just to tell the user to use the Admin command but I think it accomplishes all of our goals.
1) Deprecate FateCommand for removal to stop reading SiteConfig
2) Provide full functionality of Fate operations in the Admin command.
3) Expose the new options (cancel & summary) in Shell FateCommand to user, directing them to Admin
I will merge this PR and open a follow on in order to determine when to drop `FateAdmin`.
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248443393
> They need to exist at the same time for a deprecation warning/message to be useful to users.
That's a good point. Do you think we should just move the logic from the shell to the Admin now? That will probably be more work then creating an admin command to call the FateCommand. Either way, having it in two places for this version is significant.
--
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] EdColeman commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1246987454
The newlines in the FateSummaryReport were specifically included to visually break up the sections to make it easier for an operator to visually scan and pull out relevant information.
--
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] EdColeman commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248028328
There is a section in the docs (fate.md) that will need to be updated when these changes are merged.
--
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 diff in pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r971047539
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -363,19 +267,14 @@ public boolean failTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs, Zo
@Override
public String description() {
- return "manage FATE transactions";
+ return "WARNING: This command is deprecated for removal";
Review Comment:
```suggestion
return "manage FATE transactions (WARNING: This command is deprecated for removal)";
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -198,6 +210,24 @@ static class RestoreZooCommand {
String file;
}
+ @Parameters(commandNames = "fate",
+ commandDescription = "Operations performed on the Manager FaTE system.")
+ static class FateOpsCommand {
+ @Parameter(names = {"-c", "--cancel"},
+ description = "<txId>{ <txId>...} Cancel new or submitted FaTE transactions")
+ List<String> txIdList = new ArrayList<>();
+
+ @Parameter(names = "--summary", description = "Print a summary of FaTE transaction information")
+ boolean summarize = false;
+
+ @Parameter(names = {"-j", "--json"}, description = "Print Operations in json")
Review Comment:
Should "Operations" be "transaction". I don't think we call them operations anywhere.
--
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] milleruntime merged pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime merged PR #2914:
URL: https://github.com/apache/accumulo/pull/2914
--
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] EdColeman commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1240005527
I guess I used it because it was available and used it when testing the command line - it looks like without it there is a missing config issue that would need to be resolved it it is not to be used.
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1252765038
> Yes - I would like cancel and summary to be available - or have a strong pointer to the admin command.
So the Shell can't call Admin directly because they are in different packages. If we keep the new options in the Shell, I could print the new Admin command for the user to run if they try running the new options in the Shell. Although this feels kinda sneaky, it would look something like:
<pre>
root@uno> fate -cancel 3cb69aefefe6b6d0
2022-09-20T14:18:48,053 [shell.Shell] WARN : WARNING: This command is deprecated for removal. Use 'accumulo admin'
Run 'accumulo admin fate -c 3cb69aefefe6b6d0'
</pre>
Another option would be to move the code for the new options to the `AdminUtil` since that is in core and then have them both call `AdminUtil`, similar to the other options.
--
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 diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r975550997
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private final String warning =
Review Comment:
make static?
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1252699352
I manual tested these changes using Uno and it works as expected.
--
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 pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248814904
> > They need to exist at the same time for a deprecation warning/message to be useful to users.
>
> That's a good point. Do you think we should just move the logic from the shell to the Admin now? That will probably be more work then creating an admin command to call the FateCommand. Either way, having it in two places for this version is significant.
We still have a FateAdmin command that lives alongside FateCommand for the shell. I figure just move FateAdmin into the Admin command, add any missing actions that were added to the shell's FateCommand, then change FateCommand so its description says it's deprecated. If it's easier to make them share code, fine. Either way, we should target deletion of the shell FateCommand in 3.0, so as long as FateCommand is deprecated, and Admin's "fate" command is fully functional, it should be enough.
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1249709978
Looking for opinions on how the Admin `fate -list` and `fate -print` commands should function... The original FateAdmin (which will be dropped with this change) and the shell in 1.10, only allowed printing all transactions. In the current version, we added the ability to print one or many transactions (making `list` and `print` identical). Moving to Admin, I think we need 2 different commands. We could keep print functioning the same way and make list take one or many txIds. But grammatically, I think it makes more sense to have the new `list` command list all transactions and have `print` print information about about one or more txIds. Thoughts?
--
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] milleruntime commented on a diff in pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r971078163
##########
server/base/src/main/java/org/apache/accumulo/server/util/Admin.java:
##########
@@ -198,6 +210,24 @@ static class RestoreZooCommand {
String file;
}
+ @Parameters(commandNames = "fate",
+ commandDescription = "Operations performed on the Manager FaTE system.")
+ static class FateOpsCommand {
+ @Parameter(names = {"-c", "--cancel"},
+ description = "<txId>{ <txId>...} Cancel new or submitted FaTE transactions")
+ List<String> txIdList = new ArrayList<>();
+
+ @Parameter(names = "--summary", description = "Print a summary of FaTE transaction information")
+ boolean summarize = false;
+
+ @Parameter(names = {"-j", "--json"}, description = "Print Operations in json")
Review Comment:
I was referring more generically to the overall command. But now I realize that we probably won't print anything else in json exception the transaction so that makes more sense.
--
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] EdColeman commented on a diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r975622827
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private static final String warning =
+ "WARNING: This command is deprecated for removal. Use 'accumulo admin'\n";
Review Comment:
```suggestion
"WARNING: This command is deprecated for removal. Use 'accumulo admin' and to use new options\n";
```
--
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] EdColeman commented on a diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r975669767
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private static final String warning =
+ "WARNING: This command is deprecated for removal. Use 'accumulo admin'\n";
Review Comment:
Suggested the line read ` "WARNING: This command is deprecated for removal. Use 'accumulo admin and to use new options'\n";`
The wording is awkward - but could not think of anything better , but short - this along with the other suggestions, I'm trying to point people to the new options (if the functions are not made available in the current command)
--
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] milleruntime commented on a diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r975608935
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private final String warning =
Review Comment:
Fixed in [5be3d7c](https://github.com/apache/accumulo/pull/2914/commits/5be3d7caf039bccecd4539d8ffbda331d3b32132)
--
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] EdColeman commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248089644
I am not sure that we should have two versions of the same command that use separate options in this release. The current command should be deprecated, but could still support the additions made in 2.1.
Having the new options available in the deprecated version seems like it could easy discovery of the new options and serve as a transition. If someone is using the "deprecated" version - they would see the warning, but they may not realize that there are new options available - I guess that adding to the deprecation warning message that new options are available on in the admin version could help - even better would be if the deprecated version help listed the new options as "unavailable" - but it may be easier just to keep the two versions in sync until the deprecated version is removed.
As far as the removing line breaks in the summary - I feel they increase readability, but that is a personal preference and can live with it either way.
--
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] milleruntime commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248103420
> I am not sure that we should have two versions of the same command that use separate options in this release. The current command should be deprecated, but could still support the additions made in 2.1.
I could just close this for 2.1. Where do you suggest to move the functionality then?
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1251319091
Thanks for the ideas. I think I figured it out. If I make the "main" command for JCommander the list of Txs, then all the rest can be booleans. I will just have to add some validation since it won't make sense to just have the only options be a list of TXs.
--
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] EdColeman commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1252713792
Yes - I would like cancel and summary to be available - or have a strong pointer to the admin command.
--
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] milleruntime commented on a diff in pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#discussion_r975649507
##########
shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java:
##########
@@ -79,8 +67,8 @@
* Manage FATE transactions
*/
public class FateCommand extends Command {
-
- private final static Logger LOG = LoggerFactory.getLogger(FateCommand.class);
+ private static final String warning =
+ "WARNING: This command is deprecated for removal. Use 'accumulo admin'\n";
Review Comment:
Was there more to your suggestion? I am only seeing " and to use new options".
--
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] milleruntime commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1248160204
> I am not sure that we should have two versions of the same command that use separate options in this release. The current command should be deprecated, but could still support the additions made in 2.1.
>
> Having the new options available in the deprecated version seems like it could easy discovery of the new options and serve as a transition. If someone is using the "deprecated" version - they would see the warning, but they may not realize that there are new options available - I guess that adding to the deprecation warning message that new options are available on in the admin version could help - even better would be if the deprecated version help listed the new options as "unavailable" - but it may be easier just to keep the two versions in sync until the deprecated version is removed.
I think you are right that it will be confusing to split the command up between the shell and the admin command. I think it will be better to just deprecate the command here and then move it completely to Admin in the next version.
--
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] EdColeman commented on pull request #2914: Deprecate FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1249754826
Would something like
```
fate -list --all or
fate -list tx1 tx2 ....
```
work? And prick either list or print?
--
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] milleruntime commented on pull request #2914: Move new FateCommand options to Admin
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2914:
URL: https://github.com/apache/accumulo/pull/2914#issuecomment-1247001403
> The newlines in the FateSummaryReport were specifically included to visually break up the sections to make it easier for an operator to visually scan and pull out relevant information.
Right. There were too many line breaks when it gets called and printed to standard out. I think the extra spaces for indents helps break up the sections fine. Here is what gets printed from Admin:
<pre>
Report Time: 2022-09-14T16:12:00Z
Status counts:
IN_PROGRESS: 2
Command counts:
TABLE_COMPACT: 2
Step counts:
CompactionDriver: 2
Fate transactions (oldest first):
Status Filters: [NONE]
Running txn_id Status Command Step (top) locks held:(table id, name) locks waiting:(table id, name)
0:00:16 70bdabd00c24a451 IN_PROGRESS TABLE_COMPACT CompactionDriver held:[R:(+default,ns:), R:(1,t:ci)] waiting:[]
0:00:07 5de86f2aefb1b8b6 IN_PROGRESS TABLE_COMPACT CompactionDriver held:[R:(+default,ns:), R:(1,t:ci)] waiting:[]
</pre>
I could add blank lines between sections. What do you think?
--
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