[GitHub] [hive] veghlaci05 commented on a diff in pull request #4098: HIVE-27122: Use Caffeine for caching metadata objects in Compactor threads

["veghlaci05 (via GitHub)" <gi...@apache.org>]

veghlaci05 commented on code in PR #4098:
URL: https://github.com/apache/hive/pull/4098#discussion_r1127815441


##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MetaStoreCompactorThread.java:
##########
@@ -66,12 +66,7 @@ public void init(AtomicBoolean stop) throws Exception {
   }
 
   @Override Table resolveTable(CompactionInfo ci) throws MetaException {
-    try {
-      return getMSForConf(conf).getTable(getDefaultCatalog(conf), ci.dbname, ci.tableName);
-    } catch (MetaException e) {
-      LOG.error("Unable to find table " + ci.getFullTableName() + ", " + e.getMessage());
-      throw e;
-    }
+    return CompactorUtil.resolveTable(conf, ci.dbname, ci.tableName);

Review Comment:
   resolveTable can be removed from here and CompactorUtil.resolveTable can be called directly



##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MetadataCache.java:
##########
@@ -17,32 +17,27 @@
  */
 package org.apache.hadoop.hive.ql.txn.compactor;
 
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
 import org.apache.thrift.TBase;
 
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
+import java.util.function.Function;
 
 public class MetadataCache {
 
   private Cache<String, TBase> metaCache;
 
   public MetadataCache(boolean tableCacheOn) {

Review Comment:
   Both MetadataCache type and tableCacheOn param could have a more generic name, as this class is not specific to Metadata/tables at all.



##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MetadataCache.java:
##########
@@ -17,32 +17,27 @@
  */
 package org.apache.hadoop.hive.ql.txn.compactor;
 
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
 import org.apache.thrift.TBase;
 
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
+import java.util.function.Function;
 
 public class MetadataCache {
 
   private Cache<String, TBase> metaCache;
 
   public MetadataCache(boolean tableCacheOn) {
     if (tableCacheOn) {
-      metaCache = CacheBuilder.newBuilder().softValues().build();
+      metaCache = Caffeine.newBuilder().softValues().build();
     }
   }
 
-  public <T extends TBase<T,?>> T computeIfAbsent(String key, Callable<T> callable) throws Exception {
+  public <T extends TBase<T,?>> T computeIfAbsent(String key, Function<? super String, ? extends TBase> function) throws Exception {
     if (metaCache != null) {
-      try {
-        return (T) metaCache.get(key, callable);
-      } catch (ExecutionException e) {
-        throw (Exception) e.getCause();
-      }

Review Comment:
   Why did you replace the exception handling with a Functional interface? Is there any benefit from it, because now the client code must wrap each call.



##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/handler/TaskHandler.java:
##########
@@ -58,12 +55,7 @@ public abstract class TaskHandler {
   public abstract List<Runnable> getTasks() throws MetaException;
 
   protected Table resolveTable(String dbName, String tableName) throws MetaException {
-    try {
-      return getMSForConf(conf).getTable(getDefaultCatalog(conf), dbName, tableName);
-    } catch (MetaException e) {
-      LOG.error("Unable to find table {}.{}, {}", dbName, tableName, e.getMessage());
-      throw e;
-    }
+    return CompactorUtil.resolveTable(conf, dbName, tableName);

Review Comment:
   For now it is fine to leave it there. But this method is only required because CompactorUtil methods are static. We need to get rid of these static utility methods, so we will be able to mock `CompactorUtil.resolveTable` directly and remove this method.



##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorUtil.java:
##########
@@ -74,6 +77,40 @@ static Runnable unchecked(ThrowingRunnable<?> r) {
     }
   }
 
+  /**
+   * An interface to create {@link Function} which can throw exceptions.
+   * This is especially useful when we pass lambda-expressions which embed functions which throw exceptions
+   * @param <T> the type of the input to the function
+   * @param <R> the type of the result of the function
+   * @param <E> the exception that is being thrown by the function
+   */
+  public interface ThrowingFunction<T, R, E extends Exception> {

Review Comment:
   I'm not sure this interface is needed. `R apply(T t)` is called only from `unchecked`, and all `unchecked` calls are done for `org.apache.hadoop.hive.ql.txn.compactor.MetadataCache#computeIfAbsent` from which it was extracted. I don't get the point why is this better, it just makes the computeIfAbsent call more complicated.



-- 
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: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org