apache
Deprecate FateCommand
- Deprecate FateCommand by adding warning to usage
- Drop recently added tests from FateCommandTest
- Supports #2215
- See discussion on #2780 about problems trying to complete #2215
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.
@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().
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.
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.
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.
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:
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:[]
I could add blank lines between sections. What do you think?
There is a section in the docs (fate.md) that will need to be updated when these changes are merged.
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.
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?
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.
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.
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?
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.
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.
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.
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?
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.
Would something like
fate -list --all or
fate -list tx1 tx2 ....
work? And pick either list or print?
fate -list all
fate -list tx1 tx2 tx3
Make all a reserved word effectively for the list command?
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.
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.
@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.
Yes - I would like cancel and summary to be available - or have a strong pointer to the admin command.
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:
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'
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.
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'
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.
- Deprecate FateCommand for removal to stop reading SiteConfig
- Provide full functionality of Fate operations in the Admin command.
- 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.