[GitHub] [hbase] virajjasani commented on a change in pull request #2323: [HBASE-24957] ZKTableStateClientSideReader#isDisabledTable doesn't check if table exists or not.

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

virajjasani commented on a change in pull request #2323:
URL: https://github.com/apache/hbase/pull/2323#discussion_r479732610



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableStateClientSideReader.java
##########
@@ -175,15 +176,18 @@ static boolean isTableState(final ZooKeeperProtos.Table.State expectedState,
   /**
    * @param zkw ZooKeeperWatcher instance to use
    * @param tableName table we're checking
-   * @return Null or {@link ZooKeeperProtos.Table.State} found in znode.
+   * @return {@link ZooKeeperProtos.Table.State} found in znode.
    * @throws KeeperException
+   * @throws TableNotFoundException if tableName doesn't exist
    */
   static ZooKeeperProtos.Table.State getTableState(final ZooKeeperWatcher zkw,
       final TableName tableName)
-      throws KeeperException, InterruptedException {
+      throws KeeperException, InterruptedException, TableNotFoundException {
     String znode = ZKUtil.joinZNode(zkw.tableZNode, tableName.getNameAsString());
     byte [] data = ZKUtil.getData(zkw, znode);
-    if (data == null || data.length <= 0) return null;
+    if (data == null || data.length <= 0) {
+      throw new TableNotFoundException(tableName);

Review comment:
       I think this is good move but since so many methods of this class have started throwing TNFE, all callers of all these methods have handled change accordingly? Like HBaseFsck etc?

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
##########
@@ -4222,7 +4222,14 @@ public String explainFailure() throws IOException {
 
       @Override
       public boolean evaluate() throws IOException {
-        return getHBaseAdmin().tableExists(tableName) && getHBaseAdmin().isTableEnabled(tableName);
+        boolean tableEnabled = false;
+        try {
+          tableEnabled = getHBaseAdmin().tableExists(tableName)
+              && getHBaseAdmin().isTableEnabled(tableName);
+        } catch (TableNotFoundException tnfe) {
+          // Ignore TNFE
+        }
+        return tableEnabled;

Review comment:
       This could be
   ```
         public boolean evaluate() throws IOException {
           try {
             return getHBaseAdmin().tableExists(tableName)
                 && getHBaseAdmin().isTableEnabled(tableName);
           } catch (TableNotFoundException tnfe) {
             return false;
           }
         }
   ```




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