[GitHub] [hbase] saintstack commented on a change in pull request #2759: HBASE-25356 HBaseAdmin#getRegion() needs to filter out non-regionName…

[GitBox <gi...@apache.org>]

saintstack commented on a change in pull request #2759:
URL: https://github.com/apache/hbase/pull/2759#discussion_r540410317



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
##########
@@ -279,7 +279,9 @@ public static HRegionLocation getRegionLocation(Connection connection, byte[] re
       parsedInfo = parseRegionInfoFromRegionName(regionName);
       row = getMetaKeyForRegion(parsedInfo);
     } catch (Exception parseEx) {
-      // Ignore. This is used with tableName passed as regionName.
+      // If it is not a valid regionName(i.e, tableName), it needs to return null here
+      // as querying meta table wont help.

Review comment:
       Worth noting at trace level? Might be fun to enable trace logging on this class and see what users are sending in?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
##########
@@ -1935,6 +1936,22 @@ protected Void postOperationResult(final Void result, final long deadlineTs)
     }
   }
 
+  private boolean isHexChar(char c) {
+    return (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f') || (c >= '0' && c <= '9');

Review comment:
       There is Bytes.isHexDigit.  It is missing (c >= 'a' && c <= 'f'). Bytes might be better place for this than here?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
##########
@@ -1935,6 +1936,22 @@ protected Void postOperationResult(final Void result, final long deadlineTs)
     }
   }
 
+  private boolean isHexChar(char c) {
+    return (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f') || (c >= '0' && c <= '9');

Review comment:
       I tried poking around.. you'd think this sort of thing would be available in a lib... but I couldn't find an easy ref after looking for 5 mins.

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
##########
@@ -1935,6 +1936,22 @@ protected Void postOperationResult(final Void result, final long deadlineTs)
     }
   }
 
+  private boolean isHexChar(char c) {
+    return (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f') || (c >= '0' && c <= '9');
+
+  }
+  private boolean isMd5Hash(String encodedRegionName) {

Review comment:
       Yeah, this should be elsewhere too (RegionInfo as a static since that is where you get the MD5 length from?)... Capitalize the 'd'.... so MD5, not md5.




----------------------------------------------------------------
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