> On Oct. 17, 2016, 12:47 p.m., Barna Zsombor Klara wrote: > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java, > > line 648 > > <https://reviews.apache.org/r/52923/diff/1/?file=1539174#file1539174line648> > > > > What is the reason for this null check?
In the original version when we read a lock from zookeeper we set the clinetIp of the lock to the current server's clientIp. If I leave it as it is, then we could not differentiate between the lock created by this server, and locks created by other HS2 servers connected to this zookeeper instance. I am not sure why we set clientIp at all, since it could come from the zookeeper server and we use it only for display purposes (at least where I found reference for it). That is why I raised the question in the review, and in the jira as well. > On Oct. 17, 2016, 12:47 p.m., Barna Zsombor Klara wrote: > > service/src/java/org/apache/hive/service/server/HiveServer2.java, line 671 > > <https://reviews.apache.org/r/52923/diff/1/?file=1539176#file1539176line671> > > > > 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. The HiveLockManager could be set by HiveConf. So I treat it as a public API, and I do not want to change it between minor releases. It seems to me there should be a better solution for these kind of problems - how to handle API changes, but this is another topic altogether :) The current implementations of HiveLockManager: - ZookeeperHiveLockManager - we added the feature for that - EmbeddedLockManager - using memory to store locks - not applicable - DBLockManager - lock methods are throwing UnsupportedOperationException - not applicable - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52923/#review152862 ----------------------------------------------------------- On Oct. 18, 2016, 9:23 a.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52923/ > ----------------------------------------------------------- > > (Updated Oct. 18, 2016, 9:23 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 > >
