[GitHub] [hbase] virajjasani commented on a change in pull request #2317: HBASE-24949 Optimize FSTableDescriptors.get to not always go to fs wh…

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

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -196,57 +197,47 @@ private static TableDescriptorBuilder createMetaTableDescriptorBuilder(final Con
         .setPriority(Coprocessor.PRIORITY_SYSTEM).build());
   }
 
-  @Override
-  public void setCacheOn() throws IOException {
-    this.cache.clear();
-    this.usecache = true;
-  }
-
-  @Override
-  public void setCacheOff() throws IOException {
-    this.usecache = false;
-    this.cache.clear();
-  }
-
   @VisibleForTesting
   public boolean isUsecache() {
     return this.usecache;
   }
 
   /**
    * Get the current table descriptor for the given table, or null if none exists.
-   *
-   * Uses a local cache of the descriptor but still checks the filesystem on each call
-   * to see if a newer file has been created since the cached one was read.
+   * <p/>
+   * Uses a local cache of the descriptor but still checks the filesystem on each call if
+   * {@link #fsvisited} is not {@code true}, i.e, we haven't done a full scan yet, to see if a newer
+   * file has been created since the cached one was read.
    */
   @Override
   @Nullable
-  public TableDescriptor get(final TableName tablename)
-  throws IOException {
+  public TableDescriptor get(TableName tableName) throws IOException {
     invocations++;
     if (usecache) {
       // Look in cache of descriptors.
-      TableDescriptor cachedtdm = this.cache.get(tablename);
+      TableDescriptor cachedtdm = this.cache.get(tableName);
       if (cachedtdm != null) {
         cachehits++;
         return cachedtdm;
       }
+      // we do not need to go to fs any more
+      if (fsvisited) {
+        return null;
+      }
     }
     TableDescriptor tdmt = null;
     try {
-      tdmt = getTableDescriptorFromFs(fs, rootdir, tablename);
+      tdmt = getTableDescriptorFromFs(fs, rootdir, tableName);
     } catch (NullPointerException e) {
-      LOG.debug("Exception during readTableDecriptor. Current table name = "
-          + tablename, e);
+      LOG.debug("Exception during readTableDecriptor. Current table name = " + tableName, e);
     } catch (TableInfoMissingException e) {
       // ignore. This is regular operation
     } catch (IOException ioe) {

Review comment:
       nit: `catch (NullPointerException | IOException e)` 

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -279,8 +270,8 @@ public TableDescriptor get(final TableName tablename)
         } else {

Review comment:
       nit: In above if condition, `continue` is no longer required.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -125,11 +125,11 @@ public static void tryUpdateMetaTableDescriptor(Configuration conf) throws IOExc
       CommonFSUtils.getRootDir(conf), null);
   }
 
-  public static void tryUpdateMetaTableDescriptor(Configuration conf, FileSystem fs, Path rootdir,
+  public static TableDescriptor tryUpdateMetaTableDescriptor(Configuration conf, FileSystem fs, Path rootdir,

Review comment:
       nit: method name can represent returning TableDescriptor, maybe something like: `updateAndGetMetaTableDesc` ?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
##########
@@ -79,7 +79,7 @@
   private final FileSystem fs;
   private final Path rootdir;
   private final boolean fsreadonly;
-  private volatile boolean usecache;
+  private final boolean usecache;

Review comment:
       Now that we only have constructor to set this, +1

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/TableDescriptors.java
##########
@@ -31,7 +31,7 @@
   /**
    * @return TableDescriptor for tablename
    */
-  TableDescriptor get(final TableName tableName) throws IOException;
+  TableDescriptor get(TableName tableName) throws IOException;

Review comment:
       Is this change needed? Do we follow any convention to not provide `final` to argument? Just asking as I am not aware of 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