You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/03/30 08:37:12 UTC
[GitHub] [hbase-operator-tools] richardantal opened a new pull request #55:
HBASE-24039
richardantal opened a new pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55
HBCK2 feature negotiation to check what commands are supported
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400921376
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +140,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
Review comment:
The above mentioned functions don't call any method in hbck so I can not do the same check in those cases.
I would stick with the original checkHBCKSupport function call that we used before.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] saintstack commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-611873158
@petersomogyi Missed your ping. Sorry.
Regards this approach, I didn't understand why when I saw the JIRA go by but the following rationale makes sense to me: "It is not that easy to decide if the command is supported or not when dealing with back-ported features." I could imagine someone doing loads of backports finding this handy so +1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r403945073
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,14 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
private boolean skipCheck = false;
+ static final Map<String, List<String> > functionNameMap = new HashMap<String, List<String>>() {{
Review comment:
nit: Capitalize the variable name.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi merged pull request #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi merged pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r408156087
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +143,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
+ if (skipCheck) {
+ LOG.info("Skipped {} command version check; 'skip' set", cmd);
+ return;
+ }
+ List<Method> methods = Arrays.asList(connection.getHbck().getClass().getDeclaredMethods());
Review comment:
I am not an expert in this topic but I think it does not makes sense to run hbck on personal computer to repair a remote HBase. The user should run hbck on the same computer where the problem is. (using ssh for example.) This way the program would get the jar with the correct version.
Anyway we can continue work on a follow up solution where we get methods from the Master itself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-605953627
retest build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] joshelser commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r406532595
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,14 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
private boolean skipCheck = false;
+ static final Map<String, List<String> > functionNameMap = new HashMap<String, List<String>>() {{
Review comment:
Can you move this Map creation into a static method, and then make it immutable (ala `Collections.unmodifiableMap()`), please?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400921552
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,11 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
private boolean skipCheck = false;
+ static final Map<String, List<String> > functionNameMap = new HashMap<String, List<String>>() {{
Review comment:
In case of in Hbck interface in org.apache.hadoop.hbase.client package, the name of the method changes. (Can be because of Deprecation or refactoring.)
Also when a new command is added and the hbck command does not equals to the method name in Hbck interface.
I will write it to checkFunctionSupported as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400841441
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,11 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
Review comment:
Can this be removed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400921552
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,11 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
private boolean skipCheck = false;
+ static final Map<String, List<String> > functionNameMap = new HashMap<String, List<String>>() {{
Review comment:
In case of in Hbck interface in org.apache.hadoop.hbase.client package, the name of the method changes. (Can be because of Deprecation or refactoring.)
I will write it to checkFunctionSupported as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-614092456
retest build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-614050022
The change looks good for me.
Could you fix these checkstyle issues? For some reason the build did not post back to GitHub. https://builds.apache.org/view/H-L/view/HBase/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/169/HTML_20Report/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r408157150
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +143,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
+ if (skipCheck) {
+ LOG.info("Skipped {} command version check; 'skip' set", cmd);
+ return;
+ }
+ List<Method> methods = Arrays.asList(connection.getHbck().getClass().getDeclaredMethods());
+ List<String> finalCmds = functionNameMap.getOrDefault(cmd, Collections.singletonList(cmd));
+ boolean supported = methods.stream().anyMatch(method -> finalCmds.contains(method.getName()));
+ if (!supported) {
+ throw new UnsupportedOperationException("Unsupported command" + cmd);
Review comment:
Thanks, I have changed the message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400840723
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +140,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
Review comment:
With the introduction of this new method can we also remove `checkHBCKSupport`? As I see it is still used in `SET_REGION_STATE`, `FILESYSTEM` and `REPLICATION` commands.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-614057013
retest build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] joshelser commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r406534504
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +143,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
+ if (skipCheck) {
+ LOG.info("Skipped {} command version check; 'skip' set", cmd);
+ return;
+ }
+ List<Method> methods = Arrays.asList(connection.getHbck().getClass().getDeclaredMethods());
Review comment:
This is only checking what is defined in `Hbck` per the local environment. This might differ that the cluster's version.
e.g. a user has their personal computer with HBase 2.2.0 on it, but their cluster they're fixing has 2.2.2 on it.
I think this is OK to start with, but we might want to push some sort of RPC to get the available Hbck methods from the Master itself (when the master, of course, is up) as a future improvement.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400921655
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,11 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
Review comment:
checkHBCKSupport is using it and I would get rid of that method yet.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-613987337
retest build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r400841353
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -102,6 +104,11 @@
private Configuration conf;
static final String [] MINIMUM_HBCK2_VERSION = {"2.0.3", "2.1.1", "2.2.0", "3.0.0"};
private boolean skipCheck = false;
+ static final Map<String, List<String> > functionNameMap = new HashMap<String, List<String>>() {{
Review comment:
Can you comment here or to the `checkFunctionSupported` method when this map needs to be updated?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] joshelser commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r409883440
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +143,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
+ if (skipCheck) {
+ LOG.info("Skipped {} command version check; 'skip' set", cmd);
+ return;
+ }
+ List<Method> methods = Arrays.asList(connection.getHbck().getClass().getDeclaredMethods());
Review comment:
We see this pretty often, actually. Folks will remain on a release line, either upstream releases or downstream re-packagings of those releases.
It would definitely be an improvement to get the discrete list of things that the server supports rather than guessing based on a version string. Like you say, that makes for good follow-on work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] richardantal commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r408158545
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +143,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
+ if (skipCheck) {
+ LOG.info("Skipped {} command version check; 'skip' set", cmd);
+ return;
+ }
+ List<Method> methods = Arrays.asList(connection.getHbck().getClass().getDeclaredMethods());
+ List<String> finalCmds = functionNameMap.getOrDefault(cmd, Collections.singletonList(cmd));
+ boolean supported = methods.stream().anyMatch(method -> finalCmds.contains(method.getName()));
+ if (!supported) {
+ throw new UnsupportedOperationException("Unsupported command" + cmd);
Review comment:
I think it is not that easy to remove the version check completely, as we talked about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-606518439
retest build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on a change in pull
request #55: HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on a change in pull request #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#discussion_r403939591
##########
File path: hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java
##########
@@ -133,6 +143,19 @@ void checkHBCKSupport(ClusterConnection connection, String cmd, String ... suppo
}
}
+ void checkFunctionSupported(ClusterConnection connection, String cmd) throws IOException {
+ if (skipCheck) {
+ LOG.info("Skipped {} command version check; 'skip' set", cmd);
+ return;
+ }
+ List<Method> methods = Arrays.asList(connection.getHbck().getClass().getDeclaredMethods());
+ List<String> finalCmds = functionNameMap.getOrDefault(cmd, Collections.singletonList(cmd));
+ boolean supported = methods.stream().anyMatch(method -> finalCmds.contains(method.getName()));
+ if (!supported) {
+ throw new UnsupportedOperationException("Unsupported command" + cmd);
Review comment:
nit: missing space from concatenation.
Error message would be better like "This HBase cluster does not support command: <cmd>". For me the original could also mean that `cmd` is an unknown 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase-operator-tools] petersomogyi commented on issue #55:
HBASE-24039
Posted by GitBox <gi...@apache.org>.
petersomogyi commented on issue #55: HBASE-24039
URL: https://github.com/apache/hbase-operator-tools/pull/55#issuecomment-609821881
@saintstack What you think about this way of compatibility check?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services