HIVE-13051 : Deadline class has numerous issues (Sergey Shelukhin, reviewed by Prasanth Jayachandran)
Project: http://git-wip-us.apache.org/repos/asf/hive/repo Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/9f61fc69 Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/9f61fc69 Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/9f61fc69 Branch: refs/heads/llap Commit: 9f61fc69b9752667ea3e6253971caa7bd3157db3 Parents: 137b238 Author: Sergey Shelukhin <[email protected]> Authored: Tue Feb 23 11:28:52 2016 -0800 Committer: Sergey Shelukhin <[email protected]> Committed: Tue Feb 23 11:28:52 2016 -0800 ---------------------------------------------------------------------- .../apache/hadoop/hive/metastore/Deadline.java | 63 +++++++++----------- .../hadoop/hive/metastore/RawStoreProxy.java | 28 +++------ .../hive/metastore/RetryingHMSHandler.java | 12 +++- .../metastore/SessionPropertiesListener.java | 9 +-- 4 files changed, 50 insertions(+), 62 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hive/blob/9f61fc69/metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java b/metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java index f29d453..71d336a 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java @@ -32,34 +32,33 @@ public class Deadline { /** * its value is init from conf, and could be reset from client. */ - private long timeout; + private long timeoutNanos; /** * it is reset before executing a method */ - private long startTime = -1; + private long startTime = NO_DEADLINE; /** * The name of public methods in HMSHandler */ private String method; - private Deadline(long timeout) { - this.timeout = timeout; + private Deadline(long timeoutMs) { + this.timeoutNanos = timeoutMs * 1000000L; } /** * Deadline object per thread. */ - private static final ThreadLocal<Deadline> DEADLINE_THREAD_LOCAL = new - ThreadLocal<Deadline>() { + private static final ThreadLocal<Deadline> DEADLINE_THREAD_LOCAL = new ThreadLocal<Deadline>() { @Override protected Deadline initialValue() { return null; } }; - static void setCurrentDeadline(Deadline deadline) { + private static void setCurrentDeadline(Deadline deadline) { DEADLINE_THREAD_LOCAL.set(deadline); } @@ -67,7 +66,7 @@ public class Deadline { return DEADLINE_THREAD_LOCAL.get(); } - static void removeCurrentDeadline() { + private static void removeCurrentDeadline() { DEADLINE_THREAD_LOCAL.remove(); } @@ -85,29 +84,14 @@ public class Deadline { * reset the timeout value of this timer. * @param timeout */ - public static void resetTimeout(long timeout) throws MetaException { - if (timeout <= 0) { + public static void resetTimeout(long timeoutMs) throws MetaException { + if (timeoutMs <= 0) { throw newMetaException(new DeadlineException("The reset timeout value should be " + - "larger than 0: " + timeout)); + "larger than 0: " + timeoutMs)); } Deadline deadline = getCurrentDeadline(); if (deadline != null) { - deadline.timeout = timeout; - } else { - throw newMetaException(new DeadlineException("The threadlocal Deadline is null," + - " please register it firstly.")); - } - } - - /** - * Check whether the timer is started. - * @return - * @throws MetaException - */ - public static boolean isStarted() throws MetaException { - Deadline deadline = getCurrentDeadline(); - if (deadline != null) { - return deadline.startTime >= 0; + deadline.timeoutNanos = timeoutMs * 1000000L; } else { throw newMetaException(new DeadlineException("The threadlocal Deadline is null," + " please register it firstly.")); @@ -118,15 +102,18 @@ public class Deadline { * start the timer before a method is invoked. * @param method */ - public static void startTimer(String method) throws MetaException { + public static boolean startTimer(String method) throws MetaException { Deadline deadline = getCurrentDeadline(); - if (deadline != null) { - deadline.startTime = System.currentTimeMillis(); - deadline.method = method; - } else { + if (deadline == null) { throw newMetaException(new DeadlineException("The threadlocal Deadline is null," + " please register it firstly.")); } + if (deadline.startTime != NO_DEADLINE) return false; + deadline.method = method; + do { + deadline.startTime = System.nanoTime(); + } while (deadline.startTime == NO_DEADLINE); + return true; } /** @@ -135,7 +122,7 @@ public class Deadline { public static void stopTimer() throws MetaException { Deadline deadline = getCurrentDeadline(); if (deadline != null) { - deadline.startTime = -1; + deadline.startTime = NO_DEADLINE; deadline.method = null; } else { throw newMetaException(new DeadlineException("The threadlocal Deadline is null," + @@ -164,14 +151,18 @@ public class Deadline { } } + private static final long NO_DEADLINE = Long.MIN_VALUE; + private void check() throws MetaException{ try { - if (startTime < 0) { + if (startTime == NO_DEADLINE) { throw new DeadlineException("Should execute startTimer() method before " + "checkTimeout. Error happens in method: " + method); } - if (startTime + timeout < System.currentTimeMillis()) { - throw new DeadlineException("Timeout when executing method: " + method); + long elapsedTime = System.nanoTime() - startTime; + if (elapsedTime > timeoutNanos) { + throw new DeadlineException("Timeout when executing method: " + method + "; " + + (elapsedTime / 1000000L) + "ms exceeds " + (timeoutNanos / 1000000L) + "ms"); } } catch (DeadlineException e) { throw newMetaException(e); http://git-wip-us.apache.org/repos/asf/hive/blob/9f61fc69/metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java b/metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java index f28e232..c5e117d 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java @@ -43,11 +43,14 @@ public class RawStoreProxy implements InvocationHandler { new MetaStoreInit.MetaStoreInitData(); private final HiveConf hiveConf; private final Configuration conf; // thread local conf from HMS + private final long socketTimeout; protected RawStoreProxy(HiveConf hiveConf, Configuration conf, Class<? extends RawStore> rawStoreClass, int id) throws MetaException { this.conf = conf; this.hiveConf = hiveConf; + this.socketTimeout = HiveConf.getTimeVar(hiveConf, + HiveConf.ConfVars.METASTORE_CLIENT_SOCKET_TIMEOUT, TimeUnit.MILLISECONDS); // This has to be called before initializing the instance of RawStore init(); @@ -91,34 +94,21 @@ public class RawStoreProxy implements InvocationHandler { @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { - Object ret = null; - boolean isTimerStarted = false; - try { + Deadline.registerIfNot(socketTimeout); + boolean isTimerStarted = Deadline.startTimer(method.getName()); try { - if (!Deadline.isStarted()) { - Deadline.startTimer(method.getName()); - isTimerStarted = true; + return method.invoke(base, args); + } finally { + if (isTimerStarted) { + Deadline.stopTimer(); } - } catch (MetaException e) { - // Deadline was not registered yet. - long timeout = HiveConf.getTimeVar(hiveConf, - HiveConf.ConfVars.METASTORE_CLIENT_SOCKET_TIMEOUT, TimeUnit.MILLISECONDS); - Deadline.registerIfNot(timeout); - Deadline.startTimer(method.getName()); - isTimerStarted = true; - } - ret = method.invoke(base, args); - - if (isTimerStarted) { - Deadline.stopTimer(); } } catch (UndeclaredThrowableException e) { throw e.getCause(); } catch (InvocationTargetException e) { throw e.getCause(); } - return ret; } public Configuration getConf() { http://git-wip-us.apache.org/repos/asf/hive/blob/9f61fc69/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java b/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java index f01849d..2fc487f 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java @@ -134,9 +134,15 @@ public class RetryingHMSHandler implements InvocationHandler { if (reloadConf || gotNewConnectUrl) { baseHandler.setConf(getActiveConf()); } - Deadline.startTimer(method.getName()); - Object object = method.invoke(baseHandler, args); - Deadline.stopTimer(); + Object object = null; + boolean isStarted = Deadline.startTimer(method.getName()); + try { + object = method.invoke(baseHandler, args); + } finally { + if (isStarted) { + Deadline.stopTimer(); + } + } return new Result(object, retryCount); } catch (javax.jdo.JDOException e) { http://git-wip-us.apache.org/repos/asf/hive/blob/9f61fc69/metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java b/metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java index d16cab0..ee96678 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java @@ -36,9 +36,10 @@ public class SessionPropertiesListener extends MetaStoreEventListener { @Override public void onConfigChange(ConfigChangeEvent changeEvent) throws MetaException { - if (changeEvent.getKey().equals(HiveConf.ConfVars.METASTORE_CLIENT_SOCKET_TIMEOUT.varname)) { - Deadline.resetTimeout(HiveConf.toTime(changeEvent.getNewValue(), TimeUnit.SECONDS, - TimeUnit.MILLISECONDS)); - } + if (changeEvent.getKey().equals(HiveConf.ConfVars.METASTORE_CLIENT_SOCKET_TIMEOUT.varname)) { + // TODO: this only applies to current thread, so it's not useful at all. + Deadline.resetTimeout(HiveConf.toTime(changeEvent.getNewValue(), TimeUnit.SECONDS, + TimeUnit.MILLISECONDS)); + } } }
