Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Siddharth Seth]

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

Ship it!


Ship It!

- Siddharth Seth


On Jan. 21, 2016, 12:27 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41377/
> ---
> 
> (Updated Jan. 21, 2016, 12:27 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 32049eb 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java e8864ae 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 3bfe35a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> a2791a1 
> 
> Diff: https://reviews.apache.org/r/41377/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Siddharth Seth]

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



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


This exception is not propagated anywhere ? In the single thread case - an 
Exception from start is propagated upwards (where I'm assuming it is handled to 
shutdown HiveServer or some such).
Thread uncaughtExceptionHandler / Callable with a ThreadPoolExecutor ?



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


Is this statement still valid ? If not, please delete or modify accordingly.


- Siddharth Seth


On Jan. 4, 2016, 11:12 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41377/
> ---
> 
> (Updated Jan. 4, 2016, 11:12 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java ffe0d9a 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java e8864ae 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 3bfe35a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> a2791a1 
> 
> Diff: https://reviews.apache.org/r/41377/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Siddharth Seth]

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

Ship it!


Other than the minor changes around the comment, and Exception propagation - 
looks good to me.

- Siddharth Seth


On Jan. 4, 2016, 11:12 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41377/
> ---
> 
> (Updated Jan. 4, 2016, 11:12 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java ffe0d9a 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java e8864ae 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 3bfe35a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> a2791a1 
> 
> Diff: https://reviews.apache.org/r/41377/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

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

(Updated Jan. 21, 2016, 12:27 a.m.)


Review request for hive, Gunther Hagleitner and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 32049eb 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java e8864ae 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
3bfe35a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
a2791a1 

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


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

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

(Updated Jan. 4, 2016, 11:12 p.m.)


Review request for hive, Gunther Hagleitner and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java ffe0d9a 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java e8864ae 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
3bfe35a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
a2791a1 

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


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

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



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 974)


Added a field comment



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 987)


The old path would just re-check the file immediately before sleep. This 
actually adds an extra wait :) 
We need to check HDFS anyway, so I think it's faster to go into IOEx path 
than to wait in cases where it won't be helpful.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1025)


Where? In case of IOEx path above it's checked before every sleep, and 
after the last sleep. In case of in-process collision it's checked once before 
and after sleep.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1032)


moved the loop into the method


- Sergey Shelukhin


On Dec. 18, 2015, 10:10 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41377/
> ---
> 
> (Updated Dec. 18, 2015, 10:10 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 36e281a 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 
>   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/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41377/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

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

(Updated Dec. 21, 2015, 9:47 p.m.)


Review request for hive, Gunther Hagleitner and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0251a6f 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 
  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/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

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


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

> On Dec. 19, 2015, 2 a.m., Siddharth Seth wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, 
> > line 89
> > 
> >
> > Why is this required ?
> > 
> > and why not in the single thread case.

some part of Tez init in Hive accesses the session. The single thread has it 
set; we just propagate it to other threads.


> On Dec. 19, 2015, 2 a.m., Siddharth Seth wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java, 
> > line 124
> > 
> >
> > Is there any part of the test which verifies that threads are being 
> > used / sessions are being setup in parallel. Not sure if there's a good way 
> > to verify that though - without setting up explicit properties for tests 
> > only.

No. Rather, I saw the order change in the other test, that's how I know it 
works :)


- Sergey


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


On Dec. 18, 2015, 10:10 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41377/
> ---
> 
> (Updated Dec. 18, 2015, 10:10 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 36e281a 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 
>   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/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41377/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Siddharth Seth]

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


TezSessionState - does that need to be synchronized on various methods like 
openSession, getSession, closeSession. 
To me, it seems like access to these variables is not thread safe.

Even something like TezSessionState.queueName. That's setup in the main thread 
in TezSessionPoolManager.setupSessionPool. Then it is accessed in separate 
threads - without being a final field, or protected by synchronzied blocks.


ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 974)


General: Some docs mentioning that this method is meant to work across 
threads and processes, and how that complicates the notifiers / sleep.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 987)


Maybe a higher interval or #of attempts, so that most files in the same JVM 
don't go into the IOException path.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1025)


Seems like this is chceked far too often. Required here. Wonder if it can 
be removed further up.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1027)


Name of the file in the log will be useful



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java (line 1032)


waitAttempts isn't actually used. Drop from the api, or use instead of the 
sleep for loop in the exception block.



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


Why is this required ?

and why not in the single thread case.



ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java (line 
118)


Is there any part of the test which verifies that threads are being used / 
sessions are being setup in parallel. Not sure if there's a good way to verify 
that though - without setting up explicit properties for tests only.


- Siddharth Seth


On Dec. 18, 2015, 10:10 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41377/
> ---
> 
> (Updated Dec. 18, 2015, 10:10 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see JIRA
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 36e281a 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 
>   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/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
> 11c0325 
> 
> Diff: https://reviews.apache.org/r/41377/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

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

(Updated Dec. 18, 2015, 10:10 p.m.)


Review request for hive, Gunther Hagleitner and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 36e281a 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 
  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/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java 
11c0325 

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


Testing
---


Thanks,

Sergey Shelukhin

Review Request 41377: HIVE-12528 don't start HS2 Tez sessions in a single thread

[Sergey Shelukhin]

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

Review request for hive, Gunther Hagleitner and Siddharth Seth.


Repository: hive-git


Description
---

see JIRA


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 31f0634 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 6e196e6 
  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 

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


Testing
---


Thanks,

Sergey Shelukhin