You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hive.apache.org by ha...@apache.org on 2020/05/03 07:48:20 UTC
[hive] branch master updated: HIVE-22737 : Concurrency:
FunctionRegistry::getFunctionInfo is static object locked (Ashutosh Chauhan
via Rajesh Balamohan)
This is an automated email from the ASF dual-hosted git repository.
hashutosh pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hive.git
The following commit(s) were added to refs/heads/master by this push:
new 2b177db HIVE-22737 : Concurrency: FunctionRegistry::getFunctionInfo is static object locked (Ashutosh Chauhan via Rajesh Balamohan)
2b177db is described below
commit 2b177db2fd71ccd602247fae87362801a9095f1a
Author: Ashutosh Chauhan <ha...@apache.org>
AuthorDate: Sat Apr 25 19:00:13 2020 -0700
HIVE-22737 : Concurrency: FunctionRegistry::getFunctionInfo is static object locked (Ashutosh Chauhan via Rajesh Balamohan)
---
.../org/apache/hadoop/hive/ql/exec/Registry.java | 25 ++++++----------------
.../results/clientpositive/llap/udf_substr.q.out | 2 +-
.../clientpositive/llap/udf_substring.q.out | 2 +-
3 files changed, 8 insertions(+), 21 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 40e9e97..6ceea2f 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 @@ public class Registry {
/**
* The mapping from expression function names to expression classes.
*/
- private final Map<String, FunctionInfo> mFunctions = new LinkedHashMap<String, FunctionInfo>();
+ private final Map<String, FunctionInfo> mFunctions = new ConcurrentHashMap<String, FunctionInfo>();
private final Set<Class<?>> builtIns = Collections.synchronizedSet(new HashSet<Class<?>>());
/**
* Persistent map contains refcounts that are only modified in synchronized methods for now,
@@ -91,6 +91,7 @@ public class Registry {
/**
* 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,11 +332,9 @@ public class Registry {
* @return
*/
public FunctionInfo getFunctionInfo(String functionName) throws SemanticException {
- lock.lock();
- try {
functionName = functionName.toLowerCase();
if (FunctionUtils.isQualifiedFunctionName(functionName)) {
- FunctionInfo functionInfo = getQualifiedFunctionInfoUnderLock(functionName);
+ FunctionInfo functionInfo = getQualifiedFunctionInfo(functionName);
addToCurrentFunctions(functionName, functionInfo);
return functionInfo;
}
@@ -348,14 +347,10 @@ public class Registry {
if (functionInfo == null) {
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 +628,7 @@ public class Registry {
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 +653,7 @@ public class Registry {
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
diff --git a/ql/src/test/results/clientpositive/llap/udf_substr.q.out b/ql/src/test/results/clientpositive/llap/udf_substr.q.out
index 00fa606..7c1a0f1 100644
--- a/ql/src/test/results/clientpositive/llap/udf_substr.q.out
+++ b/ql/src/test/results/clientpositive/llap/udf_substr.q.out
@@ -8,7 +8,7 @@ PREHOOK: type: DESCFUNCTION
POSTHOOK: query: DESCRIBE FUNCTION EXTENDED substr
POSTHOOK: type: DESCFUNCTION
substr(str, pos[, len]) - returns the substring of str that starts at pos and is of length len orsubstr(bin, pos[, len]) - returns the slice of byte array that starts at pos and is of length len
-Synonyms: mid, substring
+Synonyms: substring, mid
pos is a 1-based index. If pos<0 the starting position is determined by counting backwards from the end of str.
Example:
> SELECT substr('Facebook', 5) FROM src LIMIT 1;
diff --git a/ql/src/test/results/clientpositive/llap/udf_substring.q.out b/ql/src/test/results/clientpositive/llap/udf_substring.q.out
index d6b1c4a..7e96b79 100644
--- a/ql/src/test/results/clientpositive/llap/udf_substring.q.out
+++ b/ql/src/test/results/clientpositive/llap/udf_substring.q.out
@@ -8,7 +8,7 @@ PREHOOK: type: DESCFUNCTION
POSTHOOK: query: DESCRIBE FUNCTION EXTENDED substring
POSTHOOK: type: DESCFUNCTION
substring(str, pos[, len]) - returns the substring of str that starts at pos and is of length len orsubstring(bin, pos[, len]) - returns the slice of byte array that starts at pos and is of length len
-Synonyms: mid, substr
+Synonyms: substr, mid
pos is a 1-based index. If pos<0 the starting position is determined by counting backwards from the end of str.
Example:
> SELECT substring('Facebook', 5) FROM src LIMIT 1;