You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Yongzhi Chen <yc...@cloudera.com> on 2017/01/12 23:21:21 UTC
Review Request 55479: Improve canceling response time for acquiring
locks
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55479/
-----------------------------------------------------------
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
Re: Review Request 55479: Improve canceling response time for
acquiring locks
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55479/#review161552
-----------------------------------------------------------
Ship it!
Ship It!
- Chaoyu Tang
On Jan. 13, 2017, 5:14 p.m., Yongzhi Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55479/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 5:14 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
>
>
Re: Review Request 55479: Improve canceling response time for
acquiring locks
Posted by Yongzhi Chen <yc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55479/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 5:14 p.m.)
Review request for hive, Aihua Xu and Chaoyu Tang.
Changes
-------
New Patch fixed issues found by review
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 (updated)
-----
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
Re: Review Request 55479: Improve canceling response time for
acquiring locks
Posted by Yongzhi Chen <yc...@cloudera.com>.
> 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
>
>
Re: Review Request 55479: Improve canceling response time for
acquiring locks
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55479/#review161468
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 200)
<https://reviews.apache.org/r/55479/#comment232765>
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.
ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 1129)
<https://reviews.apache.org/r/55479/#comment232766>
nit: need a space between userFromUGI,lDrvState
ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java (line 454)
<https://reviews.apache.org/r/55479/#comment232767>
should be "Query was cancelled while acquiring locks on the underlying objects."?
ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java (line 184)
<https://reviews.apache.org/r/55479/#comment232772>
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.
ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/ZooKeeperHiveLockManager.java (line 189)
<https://reviews.apache.org/r/55479/#comment232768>
as I commented before, the DriverState class might provide this method for inspecting this state, which looks better.
- Chaoyu Tang
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
>
>