----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29807/#review72119 -----------------------------------------------------------
LGTM. Had a few questions. common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <https://reviews.apache.org/r/29807/#comment118136> It might be important to highlight in the description that not all (long running) HMS methods will timeout with this config. Only certain ones "we think take long usually"....maybe we can check with Lefty how to word this in the docs. metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118107> Why do we need to store the method name of the HMS method we are timing ? Is it just so we can print it in the exception message ? Instead of storing it here, maybe we can log the method name when the DeadlineException is caught in HMS ? metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118105> How about calling this startTimer() ? startCounting() seems to be confusing ... we're just starting a timer here... metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118108> do we really need to record the method ? metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118109> stopCounter() metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118126> null instead of "" ? metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118116> firstly -> first metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java <https://reviews.apache.org/r/29807/#comment118125> execute -> executing metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/29807/#comment118118> please move this up (alphabetically) metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/29807/#comment118121> Let's call this TEST_TIMEOUT_ENABLED metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/29807/#comment118099> can we just set it to an invalid value like -1 here ? Seems weird to initialize it to a unit test specific value in HMS class. TEST_TIMEOUT_VALUE metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/29807/#comment118120> please inline this, no need for separate method here. metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java <https://reviews.apache.org/r/29807/#comment118130> Should we log the method name here instead of storing it in Deadline ? metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java <https://reviews.apache.org/r/29807/#comment118139> nit: redundant comment metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java <https://reviews.apache.org/r/29807/#comment118091> please move this import up (alphabetically) metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java <https://reviews.apache.org/r/29807/#comment118102> nit: no point converting Throwable to an Exception. just 'throw e' should be fine. metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java <https://reviews.apache.org/r/29807/#comment118103> nit: throw e metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java <https://reviews.apache.org/r/29807/#comment118095> please use HiveConf.ConfVars.METASTORE_SERVER_LONG_RUNNING_METHOD_TIMEOUT.varname metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java <https://reviews.apache.org/r/29807/#comment118096> please use HiveConf.ConfVars.METASTORE_SERVER_LONG_RUNNING_METHOD_TIMEOUT.varname - Mohit Sabharwal On Jan. 28, 2015, 8:58 a.m., Dong Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29807/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2015, 8:58 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > HIVE-9253: MetaStore server should support timeout for long running requests > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 66f436b > metastore/src/java/org/apache/hadoop/hive/metastore/Deadline.java > PRE-CREATION > metastore/src/java/org/apache/hadoop/hive/metastore/DeadlineException.java > PRE-CREATION > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > fc6f067 > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java > 574141c > metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java > 01ad36a > > metastore/src/java/org/apache/hadoop/hive/metastore/SessionPropertiesListener.java > PRE-CREATION > metastore/src/test/org/apache/hadoop/hive/metastore/TestDeadline.java > PRE-CREATION > > metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/29807/diff/ > > > Testing > ------- > > UT passed > > > Thanks, > > Dong Chen > >
