hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Barna Zsombor Klara <zsombor.kl...@cloudera.com>
Subject Re: Review Request 52923: HIVE-14979 Removing stale Zookeeper locks at HiveServer2 initialization
Date Mon, 17 Oct 2016 12:47:03 GMT

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



Thanks for the patch Peter, looks good. I only have some questions/comments, but no concrete
issues to open.


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 1770)
<https://reviews.apache.org/r/52923/#comment222031>

    nit1: I wouldn't use a semi-colon, only a comma.
    nit2: Whether to release stale zookeeper locks at HiveServer2 startup time which *were*
    nit3: release every *lock* created by these servers too



ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java (line
648)
<https://reviews.apache.org/r/52923/#comment222032>

    What is the reason for this null check?



ql/src/test/org/apache/hadoop/hive/ql/lockmgr/zookeeper/TestZookeeperLockManager.java (line
68)
<https://reviews.apache.org/r/52923/#comment222029>

    Do we still need this infinite loop? What if we get something more permanent than a transient
bind exception, should we retry for every exception and for inifity?



service/src/java/org/apache/hive/service/server/HiveServer2.java (line 670)
<https://reviews.apache.org/r/52923/#comment222028>

    Any reason we are biased towards the ZooKeeperHiveLockManager? I would maybe call the
method releaseStaleLocks, put it into the HiveLockManager interface and then leave it to the
implementation to see if those are identified by IP and released per HS2 instance or dealt
with otherwise.


- Barna Zsombor Klara


On Oct. 17, 2016, 9:34 a.m., Peter Vary wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52923/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2016, 9:34 a.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Marta Kuczora, Miklos Csanady, namit jain,
Sergio Pena, and Barna Zsombor Klara.
> 
> 
> Bugs: HIVE-14979
>     https://issues.apache.org/jira/browse/HIVE-14979
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Adding a new configuration option to HiveConf to signal whether stale lock removal is
requested on startup.
> Adding a new method to ZooKeeperHiveLockManager to remove stale locks
> Modifying the HiveServer2 to instantiate a lock manager and call the new method if defined
by the configuration.
> 
> Please take extra care when reviewing these:
> - Modifying the lock fetching method to use the clientIp from the lock, and not update
with the current ip - Not sure why it was done before
> - Instantiation of the lock manager - I might not chose the best method for it
> 
> Open for any suggestions :)
> 
> Thanks,
> Peter
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8ffae3b 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java
14d0ef4 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/zookeeper/TestZookeeperLockManager.java
3f9926e 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 590b1f3 
> 
> Diff: https://reviews.apache.org/r/52923/diff/
> 
> 
> Testing
> -------
> 
> Created 2 unit test cases:
> - Removing own locks
> - Not removing other server's locks
> 
> Manually tested the Lock manager instantiation method on HiveServer2
> 
> 
> Thanks,
> 
> Peter Vary
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message