Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============6802097742182845169=="
MIME-Version: 1.0
Subject: Re: Review Request 52923: HIVE-14979 Removing stale Zookeeper locks at
 HiveServer2 initialization
From: Barna Zsombor Klara <zsombor.klara@cloudera.com>
To: Barna Zsombor Klara <zsombor.klara@cloudera.com>,
 Ashutosh Chauhan <hashutosh@apache.org>, Marta Kuczora <kuczoram@cloudera.com>,
 Miklos Csanady <miklos.csanady@cloudera.com>, namit jain <njain@fb.com>,
 Sergio Pena <sergio.pena@cloudera.com>
Cc: Peter Vary <pvary@cloudera.com>, hive <dev@hive.apache.org>
Date: Mon, 17 Oct 2016 12:47:03 -0000
Message-ID: <20161017124703.14471.60402@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Barna Zsombor Klara <noreply@reviews.apache.org>
References: <20161017093435.5284.5547@reviews.apache.org>
In-Reply-To: <20161017093435.5284.5547@reviews.apache.org>
Reply-To: Barna Zsombor Klara <zsombor.klara@cloudera.com>
archived-at: Mon, 17 Oct 2016 12:47:07 -0000

--===============6802097742182845169==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


--===============6802097742182845169==--