Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-23 Thread Vikram Dixit Kumaraswamy 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/#review111805
---



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
120)


Need a condition for the restart thread as well.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
455)


All of this looks a bit complicated. I still did not understand why we need 
to re-check the queue below. I think this can be simplified but now may not be 
the time for it.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
459)


Don't we need to refresh this session? It looks like the number of sessions 
in the queue can go down this way.


- Vikram Dixit Kumaraswamy


On Dec. 23, 2015, 1:30 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41431/
> ---
> 
> (Updated Dec. 23, 2015, 1:30 a.m.)
> 
> 
> Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d2dd9c6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 0d84340 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e5df2ec 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> d55c9fe 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41431/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-23 Thread Sergey Shelukhin 


> On Dec. 24, 2015, 12:02 a.m., Vikram Dixit Kumaraswamy wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 120
> > 
> >
> > Need a condition for the restart thread as well.

implied


> On Dec. 24, 2015, 12:02 a.m., Vikram Dixit Kumaraswamy wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 455
> > 
> >
> > All of this looks a bit complicated. I still did not understand why we 
> > need to re-check the queue below. I think this can be simplified but now 
> > may not be the time for it.

added comment


> On Dec. 24, 2015, 12:02 a.m., Vikram Dixit Kumaraswamy wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 460
> > 
> >
> > Don't we need to refresh this session? It looks like the number of 
> > sessions in the queue can go down this way.

closeAndReopen opens in finally


- Sergey


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/#review111805
---


On Dec. 23, 2015, 1:30 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41431/
> ---
> 
> (Updated Dec. 23, 2015, 1:30 a.m.)
> 
> 
> Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d2dd9c6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 0d84340 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e5df2ec 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> d55c9fe 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41431/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-22 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/
---

(Updated Dec. 23, 2015, 1:30 a.m.)


Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d2dd9c6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
0d84340 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e5df2ec 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
d55c9fe 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

Diff: https://reviews.apache.org/r/41431/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-21 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/
---

(Updated Dec. 22, 2015, 3:18 a.m.)


Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d2dd9c6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
0d84340 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e5df2ec 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
d55c9fe 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

Diff: https://reviews.apache.org/r/41431/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-18 Thread Sergey Shelukhin 


> On Dec. 17, 2015, 10:51 a.m., Vikram Dixit Kumaraswamy wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 129
> > 
> >
> > expiration queue length is fixed at 11? Shouldn't it be a configurable 
> > value?

that's the initial length


> On Dec. 17, 2015, 10:51 a.m., Vikram Dixit Kumaraswamy wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 245
> > 
> >
> > Given the assertion below, you should rename this method to 
> > returnSessionToPool or something like that if it is guaranteed to be a pool 
> > session or change the assertion below.

the method is on the pool, so it kind of implies you are returning to the pool 
what you have taken from the pool :)


- Sergey


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/#review110852
---


On Dec. 16, 2015, 1:57 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41431/
> ---
> 
> (Updated Dec. 16, 2015, 1:57 a.m.)
> 
> 
> Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 243f281 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 0d84340 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> d55c9fe 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41431/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-18 Thread Sergey Shelukhin 


> On Dec. 17, 2015, 10:51 a.m., Vikram Dixit Kumaraswamy wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 467
> > 
> >
> > Can you raise a jira for follow up on this?

fixed the issue


- Sergey


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/#review110852
---


On Dec. 16, 2015, 1:57 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41431/
> ---
> 
> (Updated Dec. 16, 2015, 1:57 a.m.)
> 
> 
> Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 243f281 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 0d84340 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> d55c9fe 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41431/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-18 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/
---

(Updated Dec. 19, 2015, 12:59 a.m.)


Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 60ac0c0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
0d84340 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
d55c9fe 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

Diff: https://reviews.apache.org/r/41431/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-18 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/#review111327
---



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
456)


note: probably need min(0, ...)


- Sergey Shelukhin


On Dec. 19, 2015, 12:59 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41431/
> ---
> 
> (Updated Dec. 19, 2015, 12:59 a.m.)
> 
> 
> Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 60ac0c0 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 0d84340 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> d55c9fe 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41431/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-18 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/
---

(Updated Dec. 19, 2015, 1:03 a.m.)


Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 60ac0c0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
0d84340 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
d55c9fe 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

Diff: https://reviews.apache.org/r/41431/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-17 Thread Vikram Dixit Kumaraswamy 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/#review110852
---



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
71)


Nit: Whitespace at the end of the declaration needs to be removed.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
127)


expiration queue length is fixed at 11? Shouldn't it be a configurable 
value?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
235)


Given the assertion below, you should rename this method to 
returnSessionToPool or something like that if it is guaranteed to be a pool 
session or change the assertion below.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
247)


Change this assertion or change the method name.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
247)


Change this assertion or the method name because it does not look like this 
was the intention from the previous code.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
449)


Can you raise a jira for follow up on this?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java (line 
459)


Can you add more comments here explaining the states and how the 
interaction with the session pool occurs.


- Vikram Dixit Kumaraswamy


On Dec. 16, 2015, 1:57 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41431/
> ---
> 
> (Updated Dec. 16, 2015, 1:57 a.m.)
> 
> 
> Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 243f281 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 0d84340 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> d55c9fe 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41431/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Review Request 41431: HIVE-12674 HS2 Tez session lifetime

2015-12-15 Thread Sergey Shelukhin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41431/
---

Review request for hive, Siddharth Seth and Vikram Dixit Kumaraswamy.


Repository: hive-git


Description
---

see JIRA


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 243f281 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 971dac9 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezJobMonitor.java f6bc19c 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
0d84340 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java e1a8041 
  ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java c066c7a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
d55c9fe 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

Diff: https://reviews.apache.org/r/41431/diff/


Testing
---


Thanks,

Sergey Shelukhin