> On Jan. 13, 2017, 1:17 a.m., Chaoyu Tang wrote: > >
Enum DriverState and lock somestime work with code outside, they can not totally encapsulated. > On Jan. 13, 2017, 1:17 a.m., Chaoyu Tang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 205 > > <https://reviews.apache.org/r/55479/diff/1/?file=1604041#file1604041line205> > > > > I wonder if it might look cleaner if we have an inner class called > > DriverState similar to this LockedDriverState. But all driver state related > > stuffs such as enum, lock are encapsulated in this class. It provides the > > methods for state transition etc. The lock and state sometimes work with code outside, so it can not fully encapsulated. > On Jan. 13, 2017, 1:17 a.m., Chaoyu Tang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java, > > line 189 > > <https://reviews.apache.org/r/55479/diff/1/?file=1604049#file1604049line189> > > > > as I commented before, the DriverState class might provide this method > > for inspecting this state, which looks better. This is a simplefied condition check to mimize lock time, the strict check should be: lock() if state is not interrupt do aquirelock from zookeeper unlock() And private boolean isInterrupted() in Driver.java is different from this one. In Driver.java it interrupts current thread, here we do not So if we encapsulate the method, we lose the flexible. > On Jan. 13, 2017, 1:17 a.m., Chaoyu Tang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java, line 184 > > <https://reviews.apache.org/r/55479/diff/1/?file=1604044#file1604044line184> > > > > Race condition here: > > if hiveLocks == null was caused by the interruption, but when the code > > executes this step, the state was just changed to be interrupted, then the > > exception msg will not be right. I thought about this, in our code we sacrify race condition a little bit to improve performance. The worst case is the error message becomes: Locks on the underlying objects cannot be acquired. Other wise, the lock for driverstate has to be locked the whole acquirelock method. > On Jan. 13, 2017, 1:17 a.m., Chaoyu Tang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1134 > > <https://reviews.apache.org/r/55479/diff/1/?file=1604041#file1604041line1134> > > > > nit: need a space between userFromUGI,lDrvState The new patch will fix the issue. > On Jan. 13, 2017, 1:17 a.m., Chaoyu Tang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java, line 454 > > <https://reviews.apache.org/r/55479/diff/1/?file=1604042#file1604042line454> > > > > should be "Query was cancelled while acquiring locks on the underlying > > objects."? The new patch will fix the issue. - Yongzhi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55479/#review161468 ----------------------------------------------------------- On Jan. 12, 2017, 11:21 p.m., Yongzhi Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55479/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2017, 11:21 p.m.) > > > Review request for hive, Aihua Xu and Chaoyu Tang. > > > Bugs: HIVE-15572 > https://issues.apache.org/jira/browse/HIVE-15572 > > > Repository: hive-git > > > Description > ------- > > 1. Add data structure to pass driverstate > 2. Driver state check when acquire locks by zookeeper. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java > fd6020b85591ea190aa33ae9f2dc925a38fc7471 > ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java > 721974db03f1f29bdb84f41db317e37a6a78ca32 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java > 45ead16560ce7514a1ab6f4ac2de6771582a8a73 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java > 24fbd9af5fb7be6b238c6ed246e360477d3c47de > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/EmbeddedLockManager.java > 20e114776f143715d5820e6a1acb794a9d6de02c > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockManager.java > b2eb99775c220e9ce347fa1cb918ebf4e738eac2 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java > ce220a21de01a188da940e4511ee6876d0c15a4a > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManagerImpl.java > ed022d9193f14436ed527f9cbd3df45d48857cf4 > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java > 14d0ef4e27e0518c1bafcbdcde12f09e101a3321 > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDummyTxnManager.java > e189d383b6d090ce151b6ab30fb240c261430239 > > Diff: https://reviews.apache.org/r/55479/diff/ > > > Testing > ------- > > Unit test > Manual test > > > Thanks, > > Yongzhi Chen > >
