From 373fcdc9c2a9c3c59ed61fa815586dda098985ad Mon Sep 17 00:00:00 2001 From: Ashutosh Chauhan Date: Sat, 25 Apr 2020 19:00:13 -0700 Subject: [PATCH] HIVE-22737 : Concurrency: FunctionRegistry::getFunctionInfo is static object locked --- .../apache/hadoop/hive/ql/exec/Registry.java | 34 +++++-------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java index 40e9e97890..3e26df16f8 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/Registry.java @@ -78,7 +78,7 @@ /** * The mapping from expression function names to expression classes. */ - private final Map mFunctions = new LinkedHashMap(); + private final Map mFunctions = new ConcurrentHashMap(); private final Set> builtIns = Collections.synchronizedSet(new HashSet>()); /** * Persistent map contains refcounts that are only modified in synchronized methods for now, @@ -91,6 +91,7 @@ /** * The epic lock for the registry. This was added to replace the synchronized methods with * minimum disruption; the locking should really be made more granular here. + * This lock is protecting mFunctions, builtIns and persistent maps. */ private final ReentrantLock lock = new ReentrantLock(); @@ -331,31 +332,22 @@ private void validateClass(Class input, Class expected) { * @return */ public FunctionInfo getFunctionInfo(String functionName) throws SemanticException { - lock.lock(); - try { functionName = functionName.toLowerCase(); - if (FunctionUtils.isQualifiedFunctionName(functionName)) { - FunctionInfo functionInfo = getQualifiedFunctionInfoUnderLock(functionName); - addToCurrentFunctions(functionName, functionInfo); - return functionInfo; - } // First try without qualifiers - would resolve builtin/temp functions. - // Otherwise try qualifying with current db name. FunctionInfo functionInfo = mFunctions.get(functionName); if (functionInfo != null && functionInfo.isBlockedFunction()) { throw new SemanticException ("UDF " + functionName + " is not allowed"); } if (functionInfo == null) { - functionName = FunctionUtils.qualifyFunctionName( + if (!FunctionUtils.isQualifiedFunctionName(functionName)) { + // try qualifying with current db name. + functionName = FunctionUtils.qualifyFunctionName( functionName, SessionState.get().getCurrentDatabase().toLowerCase()); - functionInfo = getQualifiedFunctionInfoUnderLock(functionName); + } + functionInfo = getQualifiedFunctionInfo(functionName); } addToCurrentFunctions(functionName, functionInfo); return functionInfo; - } finally { - lock.unlock(); - } - } private void addToCurrentFunctions(String functionName, FunctionInfo functionInfo) { @@ -633,7 +625,7 @@ public GenericUDAFResolver getGenericUDAFResolver(String functionName) throws Se return null; } - private FunctionInfo getQualifiedFunctionInfoUnderLock(String qualifiedName) throws SemanticException { + private FunctionInfo getQualifiedFunctionInfo(String qualifiedName) throws SemanticException { FunctionInfo info = mFunctions.get(qualifiedName); if (info != null && info.isBlockedFunction()) { throw new SemanticException ("UDF " + qualifiedName + " is not allowed"); @@ -658,15 +650,7 @@ private FunctionInfo getQualifiedFunctionInfoUnderLock(String qualifiedName) thr if (conf == null || !HiveConf.getBoolVar(conf, ConfVars.HIVE_ALLOW_UDF_LOAD_ON_DEMAND)) { return null; } - // This is a little bit weird. We'll do the MS call outside of the lock. Our caller calls us - // under lock, so we'd preserve the lock state for them; their finally block will release the - // lock correctly. See the comment on the lock field - the locking needs to be reworked. - lock.unlock(); - try { - return getFunctionInfoFromMetastoreNoLock(qualifiedName, conf); - } finally { - lock.lock(); - } + return getFunctionInfoFromMetastoreNoLock(qualifiedName, conf); } // should be called after session registry is checked -- 2.17.2 (Apple Git-113)