Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-31 Thread Sergey Shelukhin 

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

(Updated Oct. 31, 2017, 9:13 p.m.)


Review request for hive and Prasanth_J.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java a3c853a5c5 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestTriggersWorkloadManager.java
 fdb660af03 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
d978a25b14 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
 45c3e38dcc 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
a326db3ab0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
da93a3a791 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java b67c933b19 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
9b4714f1d7 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
613522357e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
144816862d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
81d6b859a6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
35e5710629 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
 209cf57a6a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
59efd43be6 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
258a865fef 


Diff: https://reviews.apache.org/r/63230/diff/6/

Changes: https://reviews.apache.org/r/63230/diff/5-6/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-31 Thread Sergey Shelukhin 

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

(Updated Oct. 31, 2017, 6:54 p.m.)


Review request for hive and Prasanth_J.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java a3c853a5c5 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestTriggersWorkloadManager.java
 fdb660af03 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
d978a25b14 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
 45c3e38dcc 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
a326db3ab0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
da93a3a791 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java b67c933b19 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
9b4714f1d7 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
613522357e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
144816862d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
81d6b859a6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
35e5710629 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
 209cf57a6a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
59efd43be6 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
258a865fef 


Diff: https://reviews.apache.org/r/63230/diff/5/

Changes: https://reviews.apache.org/r/63230/diff/4-5/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread j . prasanth . j 

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


Ship it!




Ship It!

- Prasanth_J


On Oct. 27, 2017, 10:10 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 10:10 p.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread Sergey Shelukhin 

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

(Updated Oct. 27, 2017, 10:10 p.m.)


Review request for hive and Prasanth_J.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
d978a25b14 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
 45c3e38dcc 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
a326db3ab0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
da93a3a791 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java b67c933b19 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
9b4714f1d7 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
613522357e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
144816862d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
81d6b859a6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
d725e90475 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
 209cf57a6a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
59efd43be6 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
258a865fef 


Diff: https://reviews.apache.org/r/63230/diff/4/

Changes: https://reviews.apache.org/r/63230/diff/3-4/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread Sergey Shelukhin 


> On Oct. 27, 2017, 6:24 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 1306 (patched)
> > 
> >
> > nit: do we know the reason why? may be print some user info from 
> > EventState (if rpName has changed/added or version changed). So its clear 
> > to the user that admin did something to RP that triggered the kill.

Updated. Some of the errors will need to be propagated when killquery usage is 
added in the followup jira


- Sergey


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


On Oct. 27, 2017, 2:43 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 2:43 a.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread j . prasanth . j 

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



JIRA is down for indexing. Mostly the changes look good. Left some minor 
comments. Esp. around the error msg when HiveException is thrown for query kill 
(more specific info will be useful for the user and debugging).

- Prasanth_J


On Oct. 27, 2017, 2:43 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 2:43 a.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread j . prasanth . j 


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 130 (patched)
> > 
> >
> > use notEmpty.awaitUninterruptibly() ? won't wake up for spurious 
> > interrupts. no need to loop with that.
> 
> Sergey Shelukhin wrote:
> Hmm... I don't like setting things to wait forever as I've seen it cause 
> bugs many times in the past. With the loop, it rechecks the pool so it's 
> harder to introduce bugs, although now it's strictly speaking unnecessary to 
> recheck

Makes sense. Will drop it. There is also a potential for lost notification 
which will cause infinite wait.


- Prasanth_J


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


On Oct. 27, 2017, 2:43 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 2:43 a.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread j . prasanth . j 

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




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java
Lines 83 (patched)


Agreed. But you can schedule a new instance of CompletableFuture that 
completeExceptionally after timeout. 
You can compose this instance of timeout CompletableFuture with other 
SettableFutures to single CompletableFuture with acceptEither.

So you either get timeout or some completed future.


- Prasanth_J


On Oct. 27, 2017, 2:43 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 2:43 a.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread j . prasanth . j 


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Line 98 (original), 110 (patched)
> > 
> >
> > would be cleaner with executor service + thread factory to name the 
> > thread based on idx or session name instead of using FutureTask directly.
> 
> Sergey Shelukhin wrote:
> this makes special casing below difficult, and doesn't seem necessary 
> since these are one-shot thread. Why create an executor service for that and 
> make sure to close it.

How do you know if timeout runnable executed or not? For debugging, better to 
have a log line in TimeoutRunnable.


- Prasanth_J


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


On Oct. 27, 2017, 2:43 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 2:43 a.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-27 Thread j . prasanth . j 

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




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1306 (patched)


nit: do we know the reason why? may be print some user info from EventState 
(if rpName has changed/added or version changed). So its clear to the user that 
admin did something to RP that triggered the kill.


- Prasanth_J


On Oct. 27, 2017, 2:43 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 27, 2017, 2:43 a.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-26 Thread Sergey Shelukhin 

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

(Updated Oct. 27, 2017, 2:43 a.m.)


Review request for hive and Prasanth_J.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 892ebfa166 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
d978a25b14 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
 45c3e38dcc 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
a326db3ab0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
da93a3a791 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java b67c933b19 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
9b4714f1d7 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
613522357e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
144816862d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
81d6b859a6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
d725e90475 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
 209cf57a6a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
59efd43be6 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
258a865fef 


Diff: https://reviews.apache.org/r/63230/diff/3/

Changes: https://reviews.apache.org/r/63230/diff/2-3/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-26 Thread Sergey Shelukhin 


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Line 88 (original), 100 (patched)
> > 
> >
> > nit: <=1 ?

there's a check on the previous line


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Line 98 (original), 110 (patched)
> > 
> >
> > would be cleaner with executor service + thread factory to name the 
> > thread based on idx or session name instead of using FutureTask directly.

this makes special casing below difficult, and doesn't seem necessary since 
these are one-shot thread. Why create an executor service for that and make 
sure to close it.


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Line 102 (original), 114 (patched)
> > 
> >
> > why this special casing here? why are invoking threadTasks[0].run() 
> > only for i== 0 and not others ?

Because this is blocking, so might as well use the current thread. It's the 
same as the old logic that has been moved.


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 120 (patched)
> > 
> >
> > Better to use ListenableFuture instead of potential blocking here 
> > during future.get()

We want to block here... old code


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 130 (patched)
> > 
> >
> > use notEmpty.awaitUninterruptibly() ? won't wake up for spurious 
> > interrupts. no need to loop with that.

Hmm... I don't like setting things to wait forever as I've seen it cause bugs 
many times in the past. With the loop, it rechecks the pool so it's harder to 
introduce bugs, although now it's strictly speaking unnecessary to recheck


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 335 (patched)
> > 
> >
> > use sequence number from znode for now? assuming restart will generate 
> > new znode and sequence number.
> > else UUID in service instance.
> > 
> > Can you add logging here to record the create/remove/update events for 
> > debugging in case if this issue happens. 
> > 
> > Please file jira for tracking this.

Filed; there's already logging there


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 347 (patched)
> > 
> >
> > make this dummy true future a static constant.

I am not sure it would be safe if multiple threads e.g. try to add listeners to 
it or otherwise try to work it into their workflow.


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 372 (patched)
> > 
> >
> > this entire block looks repeated here and for initial sessions. put 
> > this in a separate method instead? also can use single thread pool/executor 
> > service for this as mentioned in earlier comments.

The difference is that current thread is not used. Again executor is not needed 
because I want the threads to finish and go away.


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 401 (patched)
> > 
> >
> > when, how, who will drain sessions in toClose? I don't see it being 
> > drained and closed anywhere in this patch.

The caller. Added javadoc. syncWork.toDestroyNoRestart is passed as toClose by 
WM


> On Oct. 25, 2017, 9:57 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 434 (patched)
> > 
> >
> > use CountDownLatch instead? before creating CreateSessionsRunnable we 
> > know how many sessions we want to create (initial or delta). We can set 
> > that to latch and these runnables can simply decrement it after session 
> > creation. Main thread can wait for it to become zero. Easier to read and 
> > few LOC.

We actually pass deltaRemaining here during resize, so that if the pool is 
resized down concur

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-26 Thread Sergey Shelukhin 


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
> > Lines 120 (patched)
> > 
> >
> > Alternatively, since you are not looking at return value anyway. You 
> > can use CompletableFuture.allOf() which will returns one combined 
> > CompletableFuture which can be used to block.

FutureTask is not a completablefuture


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java
> > Lines 83 (patched)
> > 
> >
> > This could be doable without Guava. With JDK8 
> > CompletableFuture.acceptEither.

ScheduledFuture is not a completablefuture either... also it requires 
completionstage on top of that, which just by the length of the javadoc 
introduction seems to be designed for much more complex stuff :)


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Line 234 (original), 292 (patched)
> > 
> >
> > same here. awaitUninterruptibly()

Same reason :)


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 491 (patched)
> > 
> >
> > When will init be cancelled?
> > 
> > If admin makes changes to pool, the event gets propagated and start 
> > with init state. Now if admin makes another change immediately (before the 
> > previous one completes), would that cancel the previous init?
> > 
> > Is applying EventState synchronous?

The only way to cancel init is from this thread; see pool.update and 
pool.destroy. So, if the pool is removed, pool.destroy would always be called 
in the same iteration with setting init state to cancelled.


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 503 (patched)
> > 
> >
> > Assuming this happens when there is no capacity in cluster for spinning 
> > up new sessios (or could be other reasons)? Is there a way to determine why 
> > this failed and throw error if not restartable (may be some permission 
> > issue)?

It actually says internal error ;) This means the pool has been removed without 
init being marked as cancelled. Made the statement more specific


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 1138 (patched)
> > 
> >
> > Create follow jira to handle this case? Killing all seems like overkill 
> > :P
> > 
> > We could kill most recently submitted queries that will satisfy delta. 
> > Or make it optional to the user to pick the policy they want (pluggable).

Will file a JIRA; this was a decision when we were discussing w/Carter and 
Gunther... because killing only some is "too complicated" :)


> On Oct. 27, 2017, 12:25 a.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 1262 (patched)
> > 
> >
> > is this a recoverable state. If not should session be set to null here?

no, not recoverable... also if state is invalid master thread cannot handle it. 
It probably means someone added an enum constant.


- Sergey


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


On Oct. 24, 2017, 11:58 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 24, 2017, 11:58 p.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0b4abb824f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/Session

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-26 Thread j . prasanth . j 

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




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 120 (patched)


Alternatively, since you are not looking at return value anyway. You can 
use CompletableFuture.allOf() which will returns one combined CompletableFuture 
which can be used to block.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java
Lines 83 (patched)


This could be doable without Guava. With JDK8 
CompletableFuture.acceptEither.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 264 (patched)


I don't see value being used anywhere. Use Guava's Set.newIdentityHashSet() 
instead?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Line 234 (original), 292 (patched)


same here. awaitUninterruptibly()



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 332 (patched)


cosmetic nit: 
() -> {} lambda for runnable



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 363 (patched)


nit: improve comment to say reclaim/rebalance what?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 491 (patched)


When will init be cancelled?

If admin makes changes to pool, the event gets propagated and start with 
init state. Now if admin makes another change immediately (before the previous 
one completes), would that cancel the previous init?

Is applying EventState synchronous?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 503 (patched)


Assuming this happens when there is no capacity in cluster for spinning up 
new sessios (or could be other reasons)? Is there a way to determine why this 
failed and throw error if not restartable (may be some permission issue)?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1138 (patched)


Create follow jira to handle this case? Killing all seems like overkill :P

We could kill most recently submitted queries that will satisfy delta. Or 
make it optional to the user to pick the policy they want (pluggable).



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1223 (patched)


nit: all CAPS for enum



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1262 (patched)


is this a recoverable state. If not should session be set to null here?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1270 (patched)


Why handle unlock conditionally? handleSession* methods seems to be called 
only from here, so might as well unconditionally unlock in finally.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1289 (patched)


Both American and British English in one line. Cancel[l]ed :)



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1312 (patched)


these two lines seems to be repeating? may be encapsulate inside a 
session.clear() or session.reset() method.


- Prasanth_J


On Oct. 24, 2017, 11:58 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 24, 2017, 11:58 p.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0b4abb824f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-25 Thread j . prasanth . j 

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




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java
Lines 29 (patched)


nit: negative or null?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Line 88 (original), 100 (patched)


nit: <=1 ?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Line 98 (original), 110 (patched)


would be cleaner with executor service + thread factory to name the thread 
based on idx or session name instead of using FutureTask directly.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Line 102 (original), 114 (patched)


why this special casing here? why are invoking threadTasks[0].run() only 
for i== 0 and not others ?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 120 (patched)


Better to use ListenableFuture instead of potential blocking here during 
future.get()



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 130 (patched)


use notEmpty.awaitUninterruptibly() ? won't wake up for spurious 
interrupts. no need to loop with that.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 196 (patched)


could you add some comments here. can't understand what is going on in this 
logic.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 257 (patched)


unused variables?



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 335 (patched)


use sequence number from znode for now? assuming restart will generate new 
znode and sequence number.
else UUID in service instance.

Can you add logging here to record the create/remove/update events for 
debugging in case if this issue happens. 

Please file jira for tracking this.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 347 (patched)


make this dummy true future a static constant.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 372 (patched)


this entire block looks repeated here and for initial sessions. put this in 
a separate method instead? also can use single thread pool/executor service for 
this as mentioned in earlier comments.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 401 (patched)


when, how, who will drain sessions in toClose? I don't see it being drained 
and closed anywhere in this patch.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java
Lines 434 (patched)


use CountDownLatch instead? before creating CreateSessionsRunnable we know 
how many sessions we want to create (initial or delta). We can set that to 
latch and these runnables can simply decrement it after session creation. Main 
thread can wait for it to become zero. Easier to read and few LOC.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 179 (patched)


nit: use different name for timepool threads.


- Prasanth_J


On Oct. 24, 2017, 11:58 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 24, 2017, 11:58 p.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0b4abb824f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exe

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-24 Thread Sergey Shelukhin 

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




ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1220 (patched)


Need to finish this senten



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
Lines 1256 (patched)


need to make sure and leave a comment about who notifies the user in this 
case (via the future)


- Sergey Shelukhin


On Oct. 24, 2017, 11:58 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63230/
> ---
> 
> (Updated Oct. 24, 2017, 11:58 p.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0b4abb824f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
> d978a25b14 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
>  45c3e38dcc 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
> a326db3ab0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
> da93a3a791 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 
> b67c933b19 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
> 9b4714f1d7 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
> 613522357e 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 144816862d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
> 81d6b859a6 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
> d725e90475 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
>  209cf57a6a 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
> 59efd43be6 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
> 258a865fef 
> 
> 
> Diff: https://reviews.apache.org/r/63230/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-24 Thread Sergey Shelukhin 

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

(Updated Oct. 24, 2017, 11:58 p.m.)


Review request for hive and Prasanth_J.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 0b4abb824f 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
d978a25b14 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
 45c3e38dcc 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
a326db3ab0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
da93a3a791 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java b67c933b19 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
9b4714f1d7 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
613522357e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
144816862d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
81d6b859a6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
d725e90475 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
 209cf57a6a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
59efd43be6 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
258a865fef 


Diff: https://reviews.apache.org/r/63230/diff/2/

Changes: https://reviews.apache.org/r/63230/diff/1-2/


Testing
---


Thanks,

Sergey Shelukhin

Review Request 63230: HIVE-17841 implement applying the resource plan

2017-10-23 Thread Sergey Shelukhin 

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

Review request for hive and Prasanth_J.


Repository: hive-git


Description
---

see jira


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java a6ecb373d8 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/AmPluginNode.java 35d380c92b 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/GuaranteedTasksAllocator.java 
d978a25b14 
  
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/LlapPluginEndpointClientImpl.java
 45c3e38dcc 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/QueryAllocationManager.java 
a326db3ab0 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SessionExpirationTracker.java 
da93a3a791 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java b67c933b19 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java 
9b4714f1d7 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolSession.java 
613522357e 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
144816862d 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java 
81d6b859a6 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WmTezSession.java 00501eef93 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 
d725e90475 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java
 209cf57a6a 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/SampleTezSessionState.java 
59efd43be6 
  ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 
258a865fef 


Diff: https://reviews.apache.org/r/63230/diff/1/


Testing
---


Thanks,

Sergey Shelukhin