----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1950/#review2061 -----------------------------------------------------------
Could you explain why we want the retry logic down inside of the zookeeper-specific implementation? It seems to me that having it outside is better, since then it doesn't have to be reimplemented in other lock manager implementations as they are added. trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java <https://reviews.apache.org/r/1950/#comment4643> quorumServers is not used by this method...why has it been added here? trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java <https://reviews.apache.org/r/1950/#comment4642> But don't we still want to rethrow eventually out of this method? Here you are squelching the exception completely. - John On 2011-09-23 21:10:26, Yongqiang He wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1950/ > ----------------------------------------------------------- > > (Updated 2011-09-23 21:10:26) > > > Review request for hive and Ning Zhang. > > > Summary > ------- > > move lock retry logic into ZooKeeperHiveLockManager > > > This addresses bug HIVE-2450. > https://issues.apache.org/jira/browse/HIVE-2450 > > > Diffs > ----- > > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1171255 > trunk/conf/hive-default.xml 1171255 > trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1171255 > trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockObject.java > 1171255 > > trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java > 1171255 > > Diff: https://reviews.apache.org/r/1950/diff > > > Testing > ------- > > will run tests locally > > > Thanks, > > Yongqiang > >
