Hive
  1. Hive
  2. HIVE-2450

move lock retry logic into ZooKeeperHiveLockManager

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. HIVE-2450.1.patch
      10 kB
      He Yongqiang
    2. HIVE-2450.2.patch
      10 kB
      He Yongqiang
    3. HIVE-2450.3.patch
      10 kB
      He Yongqiang
    4. HIVE-2450.4.patch
      19 kB
      He Yongqiang

      Activity

      Hide
      jiraposter@reviews.apache.org added a comment -

      -----------------------------------------------------------
      This is an automatically generated e-mail. To reply, visit:
      https://reviews.apache.org/r/1950/
      -----------------------------------------------------------

      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

      Show
      jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1950/ ----------------------------------------------------------- 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
      Hide
      jiraposter@reviews.apache.org added a comment -

      -----------------------------------------------------------
      This is an automatically generated e-mail. To reply, visit:
      https://reviews.apache.org/r/1950/
      -----------------------------------------------------------

      (Updated 2011-09-23 21:10:26.466632)

      Review request for hive and Ning Zhang.

      Changes
      -------

      update comment in hive-default.xml

      Summary
      -------

      move lock retry logic into ZooKeeperHiveLockManager

      This addresses bug HIVE-2450.
      https://issues.apache.org/jira/browse/HIVE-2450

      Diffs (updated)


      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

      Show
      jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1950/ ----------------------------------------------------------- (Updated 2011-09-23 21:10:26.466632) Review request for hive and Ning Zhang. Changes ------- update comment in hive-default.xml Summary ------- move lock retry logic into ZooKeeperHiveLockManager This addresses bug HIVE-2450 . https://issues.apache.org/jira/browse/HIVE-2450 Diffs (updated) 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
      Hide
      jiraposter@reviews.apache.org added a comment -

      -----------------------------------------------------------
      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

      Show
      jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
      Hide
      He Yongqiang added a comment -

      address John's comments. for refactoring the retry logic out, i agree. But i think we can do it later when we do the second lock manager (even we do it now, it will need to change later).

      Show
      He Yongqiang added a comment - address John's comments. for refactoring the retry logic out, i agree. But i think we can do it later when we do the second lock manager (even we do it now, it will need to change later).
      Hide
      jiraposter@reviews.apache.org added a comment -

      -----------------------------------------------------------
      This is an automatically generated e-mail. To reply, visit:
      https://reviews.apache.org/r/1950/
      -----------------------------------------------------------

      (Updated 2011-09-26 17:56:19.501365)

      Review request for hive and Ning Zhang.

      Changes
      -------

      address comments

      Summary
      -------

      move lock retry logic into ZooKeeperHiveLockManager

      This addresses bug HIVE-2450.
      https://issues.apache.org/jira/browse/HIVE-2450

      Diffs (updated)


      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

      Show
      jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1950/ ----------------------------------------------------------- (Updated 2011-09-26 17:56:19.501365) Review request for hive and Ning Zhang. Changes ------- address comments Summary ------- move lock retry logic into ZooKeeperHiveLockManager This addresses bug HIVE-2450 . https://issues.apache.org/jira/browse/HIVE-2450 Diffs (updated) 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
      Hide
      John Sichi added a comment -

      +1. Will commit when tests pass.

      Show
      John Sichi added a comment - +1. Will commit when tests pass.
      Hide
      John Sichi added a comment -

      I got one test failure (lockneg1.q).

      Show
      John Sichi added a comment - I got one test failure (lockneg1.q).
      Hide
      He Yongqiang added a comment -

      a new patch to fix the failing testcase, and added more logging.

      One more change is that this patch removes the zookeeper instance recreation from renewZookeeperInstance. It's because once a zookeeper instance is closed, all non-persistent lock will be released. If in future we find out we do need to recreate the zookeeper instance in some cases, we will need to add logics to recreate all non-persistent node after the new zookeeper instance is created.

      Show
      He Yongqiang added a comment - a new patch to fix the failing testcase, and added more logging. One more change is that this patch removes the zookeeper instance recreation from renewZookeeperInstance. It's because once a zookeeper instance is closed, all non-persistent lock will be released. If in future we find out we do need to recreate the zookeeper instance in some cases, we will need to add logics to recreate all non-persistent node after the new zookeeper instance is created.
      Hide
      jiraposter@reviews.apache.org added a comment -

      -----------------------------------------------------------
      This is an automatically generated e-mail. To reply, visit:
      https://reviews.apache.org/r/1950/
      -----------------------------------------------------------

      (Updated 2011-09-27 21:11:09.644951)

      Review request for hive and Ning Zhang.

      Changes
      -------

      fix the failing testcase and more logging

      Summary
      -------

      move lock retry logic into ZooKeeperHiveLockManager

      This addresses bug HIVE-2450.
      https://issues.apache.org/jira/browse/HIVE-2450

      Diffs (updated)


      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/Context.java 1171255
      trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1171255
      trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockManager.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
      trunk/ql/src/test/queries/clientnegative/lockneg1.q 1171255
      trunk/ql/src/test/queries/clientnegative/lockneg2.q 1171255
      trunk/ql/src/test/queries/clientnegative/lockneg3.q 1171255
      trunk/ql/src/test/queries/clientnegative/lockneg4.q 1171255

      Diff: https://reviews.apache.org/r/1950/diff

      Testing
      -------

      will run tests locally

      Thanks,

      Yongqiang

      Show
      jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1950/ ----------------------------------------------------------- (Updated 2011-09-27 21:11:09.644951) Review request for hive and Ning Zhang. Changes ------- fix the failing testcase and more logging Summary ------- move lock retry logic into ZooKeeperHiveLockManager This addresses bug HIVE-2450 . https://issues.apache.org/jira/browse/HIVE-2450 Diffs (updated) 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/Context.java 1171255 trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java 1171255 trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockManager.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 trunk/ql/src/test/queries/clientnegative/lockneg1.q 1171255 trunk/ql/src/test/queries/clientnegative/lockneg2.q 1171255 trunk/ql/src/test/queries/clientnegative/lockneg3.q 1171255 trunk/ql/src/test/queries/clientnegative/lockneg4.q 1171255 Diff: https://reviews.apache.org/r/1950/diff Testing ------- will run tests locally Thanks, Yongqiang
      Hide
      John Sichi added a comment -

      Retesting now.

      Show
      John Sichi added a comment - Retesting now.
      Hide
      John Sichi added a comment -

      Committed to trunk. Thanks Yongqiang!

      Show
      John Sichi added a comment - Committed to trunk. Thanks Yongqiang!
      Hide
      Hudson added a comment -

      Integrated in Hive-trunk-h0.21 #980 (See https://builds.apache.org/job/Hive-trunk-h0.21/980/)
      HIVE-2450. Move retry logic into ZooKeeperHiveLockManager
      (Yongqiang He via jvs)

      jvs : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1176713
      Files :

      • /hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
      • /hive/trunk/conf/hive-default.xml
      • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Context.java
      • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java
      • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockManager.java
      • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockObject.java
      • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java
      • /hive/trunk/ql/src/test/queries/clientnegative/lockneg1.q
      • /hive/trunk/ql/src/test/queries/clientnegative/lockneg2.q
      • /hive/trunk/ql/src/test/queries/clientnegative/lockneg3.q
      • /hive/trunk/ql/src/test/queries/clientnegative/lockneg4.q
      Show
      Hudson added a comment - Integrated in Hive-trunk-h0.21 #980 (See https://builds.apache.org/job/Hive-trunk-h0.21/980/ ) HIVE-2450 . Move retry logic into ZooKeeperHiveLockManager (Yongqiang He via jvs) jvs : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1176713 Files : /hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java /hive/trunk/conf/hive-default.xml /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Context.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/Driver.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockManager.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockObject.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java /hive/trunk/ql/src/test/queries/clientnegative/lockneg1.q /hive/trunk/ql/src/test/queries/clientnegative/lockneg2.q /hive/trunk/ql/src/test/queries/clientnegative/lockneg3.q /hive/trunk/ql/src/test/queries/clientnegative/lockneg4.q

        People

        • Assignee:
          He Yongqiang
          Reporter:
          He Yongqiang
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development