ankitsinghal commented on a change in pull request #69:
URL:
https://github.com/apache/hbase-operator-tools/pull/69#discussion_r830549829
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -709,17 +766,24 @@ private static void usageSetTableState(PrintWriter
writer) {
writer.println(" An example making table name 'user' ENABLED:");
writer.println(" $ HBCK2 setTableState users ENABLED");
writer.println(" Returns whatever the previous table state was.");
+ writer.println(" If -i or --inputFiles is specified, pass one or more
input file names.");
+ writer.println(" Each file contains <TABLENAME> <STATE>, one pair per
line.);" +
+ "For example:");
+ writer.println(" $ HBCK2 -i " + SET_TABLE_STATE + " fileName1
fileName2");
}
private static void usageScheduleRecoveries(PrintWriter writer) {
- writer.println(" " + SCHEDULE_RECOVERIES + " <SERVERNAME>...");
+ writer.println(" " + SCHEDULE_RECOVERIES + "
<SERVERNAME|INPUTFILE_FOR_SERVERNAMES>...");
Review comment:
```suggestion
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.
##########
File path: hbase-hbck2/README.md
##########
@@ -280,6 +281,9 @@ Command:
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.
Review 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 <filename1> <filename2> " instead of global
"-i extraRegionsInMeta <filename1> <filename2>" .
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.
##########
File path: hbase-hbck2/README.md
##########
@@ -91,7 +91,9 @@ The above command with no options or arguments passed will
dump out the _HBCK2_
usage: HBCK2 [OPTIONS] COMMAND <ARGS>
Options:
-d,--debug run with debug output
+ -i, --inputfiles take one or more files to
read the args from
Review comment:
Nick is suggesting this @clarax
```suggestion
-i, --inputFiles take one or more files to
read the args from
```
##########
File path: hbase-hbck2/README.md
##########
@@ -173,6 +181,9 @@ Command:
$ HBCK2 extraRegionsInMeta default:table_1 ns1
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:
Review comment:
If you mean by either NAMESPACE or NAMESPACE:TABLENAME, as | looks like
a part of a format.
```suggestion
Each file contains <NAMESPACE or NAMESPACE:TABLENAME>, one per line. For
example:
```
##########
File path: hbase-hbck2/README.md
##########
@@ -137,12 +138,13 @@ Command:
Returns the pid(s) of the created AssignProcedure(s) or -1 if none.
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
Review 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.
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -733,7 +797,7 @@ private static void usageRecoverUnknown(PrintWriter writer)
{
}
private static void usageUnassigns(PrintWriter writer) {
- writer.println(" " + UNASSIGNS + " <ENCODED_REGIONNAME>...");
+ writer.println(" " + UNASSIGNS + "
<ENCODED_REGIONNAME|INPUTFILE_FOR_REGIONNAMES>...");
Review comment:
```suggestion
writer.println(" " + UNASSIGNS + " <either ENCODED_REGIONNAME or
INPUTFILE_FOR_REGIONNAMES>...");
```
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -892,17 +965,32 @@ private int doCommandLine(CommandLine commandLine,
Options options) throws IOExc
// Case handlers all have same format. Check first that the server
supports
// the feature FIRST, then move to process the command.
case SET_TABLE_STATE:
- if (commands.length < 3) {
- showErrorMessage(command + " takes tablename and state arguments:
e.g. user ENABLED");
- return EXIT_FAILURE;
+ 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(); 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)));
+ }
Review comment:
can this be combined to avoid redundant code?
##########
File path: hbase-hbck2/README.md
##########
@@ -91,7 +91,9 @@ The above command with no options or arguments passed will
dump out the _HBCK2_
usage: HBCK2 [OPTIONS] COMMAND <ARGS>
Options:
-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
Review comment:
Duplicate option defined?
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -944,31 +1032,29 @@ private int doCommandLine(CommandLine commandLine,
Options options) throws IOExc
break;
case SET_REGION_STATE:
- if (commands.length < 3) {
- showErrorMessage(command + " takes region encoded name and state
arguments: e.g. "
- + "35f30b0ce922c34bf5c284eff33ba8b3 CLOSING");
- return EXIT_FAILURE;
- }
- RegionState.State state = RegionState.State.valueOf(commands[2]);
-
- int replicaId = 0;
- String region = commands[1];
- int separatorIndex = commands[1].indexOf(",");
- if (separatorIndex > 0) {
- region = commands[1].substring(0, separatorIndex);
- replicaId = Integer.getInteger(commands[1].substring(separatorIndex
+ 1));
- }
-
- if (replicaId > 0) {
- System.out.println("Change state for replica reigon " + replicaId +
- " for primary region " + region);
- }
-
- try (ClusterConnection connection = connect()) {
- checkHBCKSupport(connection, command);
- return setRegionState(connection, region, replicaId, state);
+ 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);
+ }
Review comment:
Can this be combined to avoid redundant code?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
