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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
