> 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? > > Peter Vary wrote: > 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.
I see, thanks for the clarification. Maybe this could be mentioned in a comment, since it is not obvious at first glance. > 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. > > Peter Vary wrote: > 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 OK, didn't know the scope of the fix. However I would argue against that DBLockManager is not applicable. While some lock methods are throwing unsupported op exceptions, it has a package private lock method called from the DbTxnManager#acquireLocks. Also adding a method to the HiveLockManager interface would have the added benefit that future implementations may be able to decide on their own if this needs handling or not. But I'll leave it to you (and other reviewers) to decide. - Barna Zsombor ----------------------------------------------------------- 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 > >
