HBASE-24587 hbck2 command should accept one or more files containing … #69
Conversation
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Liked the idea of making all commands that accept list of regions to consistently also accept list of files containing the regions. Just small nit remarks to update the description to all commands that got modified here, together with the related UT testing this additional feature.
| of what a userspace encoded region name looks like. For example: | ||
| $ HBCK2 unassign 1588230740 de00010733901a05f5a2a3a382e27dd4 | ||
| Returns the pid(s) of the created UnassignProcedure(s) or -1 if none. | ||
| If -i or --inputFiles is specified, pass one or more input file names. |
There was a problem hiding this comment.
We should modify the command params description on line #274 to:
unassigns <ENCODED_REGIONNAME|-i INPUT_FILES>
There was a problem hiding this comment.
I added -i as a utility option for all commands that take a list of argument. So the syntax is
hbck -i command inputfiles
There was a problem hiding this comment.
We need still to update README for each of the modified commands. For example, assigns descriptions reads:
assigns [OPTIONS] <ENCODED_REGIONNAME/INPUTFILES_FOR_REGIONNAMES>...
But extraRegionsInMeta still reads:
extraRegionsInMeta <NAMESPACE|NAMESPACE:TABLENAME>...
However, command usage for modified commands do mention the correct parameters, so we should make README consistent.
There was a problem hiding this comment.
This new behavior is surprising to me. It's surprising because the -i is accepted as a global flag but it's arguments come after the subcommand. That's not a common feature in command line utilities that I'm familiar with. I'm not convinced this is a good behavior.
There was a problem hiding this comment.
Agreed , I think we need to define "-i " option well (and remove --inputFiles to avoid confusion), that it just alter the way the args are expected by the command, if "-i" is used and a file is not passed, a user should see an error
OR
your earlier suggestion make it local to every command so that it can be defined as "extraRegionsInMeta -i " instead of global "-i extraRegionsInMeta " .
I think if it is well defined both options are good but we can choose one and proceed, I'm more inclining also towards "local" option as we anyways have to describe them for each command. let me know your thoughts so that we can close on this.
| new FsRegionsMetaRecoverer(this.conf)) { | ||
| report = fsRegionsMetaRecoverer.reportTablesMissingRegions( | ||
| formatNameSpaceTableParam(nameSpaceOrTable)); | ||
| report = fsRegionsMetaRecoverer.reportTablesMissingRegions(getFromArgsOrFiles( |
There was a problem hiding this comment.
Missing UT and updates to reportMissingRegionsInMeta command description (as it now accepts list of files)
There was a problem hiding this comment.
Still missing UT for reportMissingRegionsInMeta. See TestHBCK2.testReportMissingRegionsInMeta.
| try (final FsRegionsMetaRecoverer fsRegionsMetaRecoverer = | ||
| new FsRegionsMetaRecoverer(this.conf)) { | ||
| List<String> namespacesTables = formatNameSpaceTableParam(commandLine.getArgs()); | ||
| List<String> namespacesTables = getFromArgsOrFiles(formatNameSpaceTableParam(commandLine.getArgs())); |
There was a problem hiding this comment.
Missing UT and updates to extraRegionsInMeta command description (as it now accepts list of files)
There was a problem hiding this comment.
Still missing UT for when files are passed as parameter. See TestHBCK2.testFormatFixExtraInMetaOneExtraSpecificTable.
| new FsRegionsMetaRecoverer(this.conf)) { | ||
| return fsRegionsMetaRecoverer.addMissingRegionsInMetaForTables( | ||
| formatNameSpaceTableParam(nameSpaceOrTable)); | ||
| return fsRegionsMetaRecoverer.addMissingRegionsInMetaForTables(getFromArgsOrFiles( |
There was a problem hiding this comment.
Missing UT and updates to addFsRegionsMissingInMeta command description (as it now accepts list of files)
There was a problem hiding this comment.
Still missing UT for the condition where files are passed as input. See TestHBCK2.testAddMissingRegionsInMetaForTables
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
We are still not testing the extra methods changed to receive a list of files as parameters. We should add this condition on UTs for _ reportTablesWithMissingRegionsInMeta_, extraRegionsInMeta, _addMissingRegionsInMetaForTables, bypass, scheduleRecoveries.
| of what a userspace encoded region name looks like. For example: | ||
| $ HBCK2 unassign 1588230740 de00010733901a05f5a2a3a382e27dd4 | ||
| Returns the pid(s) of the created UnassignProcedure(s) or -1 if none. | ||
| If -i or --inputFiles is specified, pass one or more input file names. |
There was a problem hiding this comment.
We need still to update README for each of the modified commands. For example, assigns descriptions reads:
assigns [OPTIONS] <ENCODED_REGIONNAME/INPUTFILES_FOR_REGIONNAMES>...
But extraRegionsInMeta still reads:
extraRegionsInMeta <NAMESPACE|NAMESPACE:TABLENAME>...
However, command usage for modified commands do mention the correct parameters, so we should make README consistent.
| new FsRegionsMetaRecoverer(this.conf)) { | ||
| return fsRegionsMetaRecoverer.addMissingRegionsInMetaForTables( | ||
| formatNameSpaceTableParam(nameSpaceOrTable)); | ||
| return fsRegionsMetaRecoverer.addMissingRegionsInMetaForTables(getFromArgsOrFiles( |
There was a problem hiding this comment.
Still missing UT for the condition where files are passed as input. See TestHBCK2.testAddMissingRegionsInMetaForTables
| try (final FsRegionsMetaRecoverer fsRegionsMetaRecoverer = | ||
| new FsRegionsMetaRecoverer(this.conf)) { | ||
| List<String> namespacesTables = formatNameSpaceTableParam(commandLine.getArgs()); | ||
| List<String> namespacesTables = getFromArgsOrFiles(formatNameSpaceTableParam(commandLine.getArgs())); |
There was a problem hiding this comment.
Still missing UT for when files are passed as parameter. See TestHBCK2.testFormatFixExtraInMetaOneExtraSpecificTable.
| } | ||
| String[] pidStrs = commandLine.getArgs(); | ||
| if (pidStrs == null || pidStrs.length <= 0) { | ||
| List<String> pidStrs = getFromArgsOrFiles(commandLine.getArgList()); |
There was a problem hiding this comment.
Requires UT for the new parameter option.
| } | ||
|
|
||
| List<Long> scheduleRecoveries(Hbck hbck, String[] args) throws IOException { | ||
| List<String> arglist = getFromArgsOrFiles(Arrays.asList(args)); |
There was a problem hiding this comment.
Requires UT for the new parameter option.
| new FsRegionsMetaRecoverer(this.conf)) { | ||
| report = fsRegionsMetaRecoverer.reportTablesMissingRegions( | ||
| formatNameSpaceTableParam(nameSpaceOrTable)); | ||
| report = fsRegionsMetaRecoverer.reportTablesMissingRegions(getFromArgsOrFiles( |
There was a problem hiding this comment.
Still missing UT for reportMissingRegionsInMeta. See TestHBCK2.testReportMissingRegionsInMeta.
| If -i or --inputFiles is specified, pass one or more input file names. | ||
| Each file contains encoded region names, one per line. For example: | ||
| $ HBCK2 assigns -i fileName1 fileName2 | ||
| $ HBCK2 -i assigns fileName1 fileName2 |
There was a problem hiding this comment.
The command comes after the -i? I think this was correct before the change.
There was a problem hiding this comment.
I changed it to a utility wide option like --skip
There was a problem hiding this comment.
So should this docstring be HBCK2 -i fileName1 fileName2 assigns ... ? Or maybe don't document -i here in the assigns subcommand because it's a "global" feature.
There was a problem hiding this comment.
I think the idea here is that "-i" changes how the args are expected by the command so instead of they being listed as on console, the command will accept the file instead but values/format would be different for different commands.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@clarax still working on this? |
…a list of region names/table names/namespaces
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@wchevreuil @ndimiduk Thank you for the feedback. Sorry I shelved this for other priorities. I am back to finish this. The changes are:
|
|
💔 -1 overall
This message was automatically generated. |
add unit test for the new -i option update README
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@wchevreuil @ndimiduk all ready. Please review. |
ndimiduk
left a comment
ndimiduk
left a comment
There was a problem hiding this comment.
I find this command line argument construction feels awkward to me. Did you consider a more "unix-like" approach of reading input arguments from stdin? You can detect the absence of arguments as a trigger to read from stdin, or explicitly look for the argument -. So instead of
$ hbck2 -i <subcommand> file
It might work like,
$ echo file | hbck2 <subcommand>
| usage: HBCK2 [OPTIONS] COMMAND <ARGS> | ||
| Options: | ||
| -d,--debug run with debug output | ||
| -i, --inputfiles take one or more files to read the args from |
There was a problem hiding this comment.
You need document the new argument only once. It looks like the camel-case version, inputFiles is what your parser change accepts.
There was a problem hiding this comment.
Nick is suggesting this @clarax
| -i, --inputfiles take one or more files to read the args from | |
| -i, --inputFiles take one or more files to read the args from |
| If -i or --inputFiles is specified, pass one or more input file names. | ||
| Each file contains encoded region names, one per line. For example: | ||
| $ HBCK2 assigns -i fileName1 fileName2 | ||
| $ HBCK2 -i assigns fileName1 fileName2 |
There was a problem hiding this comment.
So should this docstring be HBCK2 -i fileName1 fileName2 assigns ... ? Or maybe don't document -i here in the assigns subcommand because it's a "global" feature.
hbase-hbck2/README.md
Outdated
| -r,--recursive bypass parent and its children. SLOW! EXPENSIVE! | ||
| -w,--lockWait milliseconds to wait before giving up; default=1 | ||
| Pass one (or more) procedure 'pid's to skip to procedure finish. Parent | ||
| Pass one (or more) procedure 'pid's to skip to procedure finish. Parent |
There was a problem hiding this comment.
Unintentional whitespace change?
hbase-hbck2/README.md
Outdated
| reportMissingRegionsInMeta <NAMESPACE|NAMESPACE:TABLENAME>... | ||
| To be used when regions missing from hbase:meta but directories | ||
| To be used when regions missing from hbase:meta but directories |
There was a problem hiding this comment.
Another unintentional whitespace change?
| of what a userspace encoded region name looks like. For example: | ||
| $ HBCK2 unassign 1588230740 de00010733901a05f5a2a3a382e27dd4 | ||
| Returns the pid(s) of the created UnassignProcedure(s) or -1 if none. | ||
| If -i or --inputFiles is specified, pass one or more input file names. |
There was a problem hiding this comment.
This new behavior is surprising to me. It's surprising because the -i is accepted as a global flag but it's arguments come after the subcommand. That's not a common feature in command line utilities that I'm familiar with. I'm not convinced this is a good behavior.
|
When I implemented for assigns and unassigns, I used -i after command. But once I tried to implement for all commands, I realized it could be a global flag. Making it a flag for individual command would have to deal with different parsing and validation for every single command which requires significant refactoring, code duplication and much more unit tests. But if we foresee adding more options to individual commands down the road, it is worth to standardize various command parsing/validation logic. |
| usage: HBCK2 [OPTIONS] COMMAND <ARGS> | ||
| Options: | ||
| -d,--debug run with debug output | ||
| -i, --inputfiles take one or more files to read the args from |
There was a problem hiding this comment.
Nick is suggesting this @clarax
| -i, --inputfiles take one or more files to read the args from | |
| -i, --inputFiles take one or more files to read the args from |
| -d,--debug run with debug output | ||
| -i, --inputfiles take one or more files to read the args from | ||
| -h,--help output this help message | ||
| -i,--inputFiles take one or more encoded region names |
| If -i or --inputFiles is specified, pass one or more input file names. | ||
| Each file contains encoded region names, one per line. For example: | ||
| $ HBCK2 assigns -i fileName1 fileName2 | ||
| $ HBCK2 -i assigns fileName1 fileName2 |
There was a problem hiding this comment.
I think the idea here is that "-i" changes how the args are expected by the command so instead of they being listed as on console, the command will accept the file instead but values/format would be different for different commands.
| Returns list of extra regions for each table passed as parameter, or | ||
| for each table on namespaces specified as parameter. | ||
| If -i or --inputFiles is specified, pass one or more input file names. | ||
| Each file contains <NAMESPACE|NAMESPACE:TABLENAME>, one per line. For example: |
There was a problem hiding this comment.
If you mean by either NAMESPACE or NAMESPACE:TABLENAME, as | looks like a part of a format.
| Each file contains <NAMESPACE|NAMESPACE:TABLENAME>, one per line. For example: | |
| Each file contains <NAMESPACE or NAMESPACE:TABLENAME>, one per line. For example: |
| of what a userspace encoded region name looks like. For example: | ||
| $ HBCK2 unassign 1588230740 de00010733901a05f5a2a3a382e27dd4 | ||
| Returns the pid(s) of the created UnassignProcedure(s) or -1 if none. | ||
| If -i or --inputFiles is specified, pass one or more input file names. |
There was a problem hiding this comment.
Agreed , I think we need to define "-i " option well (and remove --inputFiles to avoid confusion), that it just alter the way the args are expected by the command, if "-i" is used and a file is not passed, a user should see an error
OR
your earlier suggestion make it local to every command so that it can be defined as "extraRegionsInMeta -i " instead of global "-i extraRegionsInMeta " .
I think if it is well defined both options are good but we can choose one and proceed, I'm more inclining also towards "local" option as we anyways have to describe them for each command. let me know your thoughts so that we can close on this.
|
|
||
| private static void usageScheduleRecoveries(PrintWriter writer) { | ||
| writer.println(" " + SCHEDULE_RECOVERIES + " <SERVERNAME>..."); | ||
| writer.println(" " + SCHEDULE_RECOVERIES + " <SERVERNAME|INPUTFILE_FOR_SERVERNAMES>..."); |
There was a problem hiding this comment.
| writer.println(" " + SCHEDULE_RECOVERIES + " <SERVERNAME|INPUTFILE_FOR_SERVERNAMES>..."); | |
| writer.println(" " + SCHEDULE_RECOVERIES + " <Either SERVERNAME OR INPUTFILE_FOR_SERVERNAMES>..."); |
As HBase expects | in some format like coprocessor so if we can be more descriptive that would be good.
|
|
||
| private static void usageUnassigns(PrintWriter writer) { | ||
| writer.println(" " + UNASSIGNS + " <ENCODED_REGIONNAME>..."); | ||
| writer.println(" " + UNASSIGNS + " <ENCODED_REGIONNAME|INPUTFILE_FOR_REGIONNAMES>..."); |
There was a problem hiding this comment.
| writer.println(" " + UNASSIGNS + " <ENCODED_REGIONNAME|INPUTFILE_FOR_REGIONNAMES>..."); | |
| writer.println(" " + UNASSIGNS + " <either ENCODED_REGIONNAME or INPUTFILE_FOR_REGIONNAMES>..."); |
| if (commands.length < 2) { | ||
| showErrorMessage(command + " takes a list of file names"); | ||
| return EXIT_FAILURE; | ||
| } | ||
| List<String> argList = getFromFiles(Arrays.asList(purgeFirst(commands))); | ||
| try (ClusterConnection connection = connect(); Hbck hbck = connection.getHbck()) { | ||
| checkFunctionSupported(connection, command); | ||
| for (String line : argList) { | ||
| String[] args = line.split("\\s+"); | ||
| System.out.println(setTableState(hbck, args)); | ||
| } | ||
| } | ||
| } | ||
| try (ClusterConnection connection = connect(); Hbck hbck = connection.getHbck()) { | ||
| checkFunctionSupported(connection, command); | ||
| System.out.println(setTableState(hbck, TableName.valueOf(commands[1]), | ||
| TableState.State.valueOf(commands[2]))); | ||
| else { | ||
| if (commands.length < 3) { | ||
| showErrorMessage(SET_TABLE_STATE + | ||
| " takes tablename and state arguments: e.g. user ENABLED"); | ||
| return EXIT_FAILURE; | ||
| } | ||
| try (ClusterConnection connection = connect(); Hbck hbck = connection.getHbck()) { | ||
| checkFunctionSupported(connection, command); | ||
| System.out.println(setTableState(hbck, purgeFirst(commands))); | ||
| } |
There was a problem hiding this comment.
can this be combined to avoid redundant code?
| if (getFromFile) { | ||
| if (commands.length < 2) { | ||
| showErrorMessage(command + " takes a list of file names"); | ||
| return EXIT_FAILURE; | ||
| } | ||
| List<String> argList = getFromFiles(Arrays.asList(purgeFirst(commands))); | ||
| try (ClusterConnection connection = connect()) { | ||
| checkHBCKSupport(connection, command); | ||
| for (String line : argList) { | ||
| String[] args = formatSetRegionStateCommand(line.split("\\s+")); | ||
| if (setRegionState(connection, args) == EXIT_FAILURE) { | ||
| showErrorMessage(command + " failed to set " + args); | ||
| } | ||
| } | ||
| } | ||
| break; | ||
| } else { | ||
| String[] args = formatSetRegionStateCommand(purgeFirst(commands)); | ||
| try (ClusterConnection connection = connect()) { | ||
| checkHBCKSupport(connection, command); | ||
| return setRegionState(connection, args); | ||
| } |
There was a problem hiding this comment.
Can this be combined to avoid redundant code?
|
🎊 +1 overall
This message was automatically generated. |
|
@ankitsinghal @wchevreuil Thank you for the feedbacks. I am closing this PR for the new patch that moves -I to individual command options. Code refactoring and unit tests are added too. Feedbacks are welcome. #105 |
6 participants
…a list of region names/table names/namespaces