Re: Review Request 55479: Improve canceling response time for acquiring locks

2017-01-13 Thread Chaoyu Tang 

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

2017-01-13 Thread Yongzhi Chen 

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

2017-01-13 Thread Yongzhi Chen 


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

2017-01-12 Thread Chaoyu Tang 

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


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)


nit: need a space between userFromUGI,lDrvState



ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java (line 454)


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)


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)


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