You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Gunther Hagleitner <gh...@hortonworks.com> on 2014/02/03 22:10:38 UTC
Re: Review Request 17471: HIVE-6325: Enable using multiple concurrent
sessions in tez
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17471/#review33496
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionState.java
<https://reviews.apache.org/r/17471/#comment62960>
I believe that file is in the wrong location. Should be in ql/test, right?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62963>
Everything is static in this class. I think it'd be better to have a singleton and non-static members. This way we could have multiple pools if desired. Also should make testing easier.
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62964>
BlockingQueue should be able to tell you length, right?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62965>
Can't sessionType denote an actual type? Class<?> is extremely general and there are no comments explaining the use.
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62961>
nit: some ws issues
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62966>
this should come from a site file not be hard coded right?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62967>
don't think this is needed
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62968>
is this the right name? shouldn't that be a yarn var?
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62969>
comment doesn't match signature
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java
<https://reviews.apache.org/r/17471/#comment62976>
It doesn't look like you're keeping track of this sessionstate here. I think we should. The user should always get/return sessions and we handle the alloc/dealloc. (why can't return close the session for non default for instance?)
service/src/java/org/apache/hive/service/server/HiveServer2.java
<https://reviews.apache.org/r/17471/#comment62977>
need to handle exception properly
- Gunther Hagleitner
On Jan. 28, 2014, 10:34 p.m., Vikram Dixit Kumaraswamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17471/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 10:34 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-6325
> https://issues.apache.org/jira/browse/HIVE-6325
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Enable using multiple concurrent sessions in tez.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 84ee78f
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 9ad5986
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionState.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java b8552a3
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionStateFactory.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezTask.java c6f431c
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java d7edda1
> ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java PRE-CREATION
> service/src/java/org/apache/hive/service/server/HiveServer2.java fa13783
>
> Diff: https://reviews.apache.org/r/17471/diff/
>
>
> Testing
> -------
>
> Added multi-threaded junit tests.
>
>
> Thanks,
>
> Vikram Dixit Kumaraswamy
>
>
Re: Review Request 17471: HIVE-6325: Enable using multiple concurrent
sessions in tez
Posted by Vikram Dixit Kumaraswamy <vi...@gmail.com>.
> On Feb. 3, 2014, 9:10 p.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, line 28
> > <https://reviews.apache.org/r/17471/diff/1/?file=453262#file453262line28>
> >
> > BlockingQueue should be able to tell you length, right?
Blocking queue has a size that reflects the number of elements in the queue not the size of the queue itself. The length can be computed using the size and the remaining capacity from the queue in a synchronized method which seems somewhat round-about to me.
> On Feb. 3, 2014, 9:10 p.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, line 65
> > <https://reviews.apache.org/r/17471/diff/1/?file=453262#file453262line65>
> >
> > don't think this is needed
Addressed above.
> On Feb. 3, 2014, 9:10 p.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, line 73
> > <https://reviews.apache.org/r/17471/diff/1/?file=453262#file453262line73>
> >
> > is this the right name? shouldn't that be a yarn var?
The tez queue name is the variable used by tez to allocate containers on a specific queue. This is again dependent on the yarn queues already. So according to the tez team we can just depend on this variable. Also, mapreduce has the behavior that its variable will return the queue 'default' if no queue is specified. We would like to choose a set of queues as default and not depend on yarn's default queue.
> On Feb. 3, 2014, 9:10 p.m., Gunther Hagleitner wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java, line 106
> > <https://reviews.apache.org/r/17471/diff/1/?file=453262#file453262line106>
> >
> > It doesn't look like you're keeping track of this sessionstate here. I think we should. The user should always get/return sessions and we handle the alloc/dealloc. (why can't return close the session for non default for instance?)
We want to retain the efficiencies in having pre-launched containers. Closing the non-default sessions would let go of that.
- Vikram
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17471/#review33496
-----------------------------------------------------------
On Jan. 28, 2014, 10:34 p.m., Vikram Dixit Kumaraswamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17471/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 10:34 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-6325
> https://issues.apache.org/jira/browse/HIVE-6325
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Enable using multiple concurrent sessions in tez.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 84ee78f
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 9ad5986
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionState.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java b8552a3
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionStateFactory.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezTask.java c6f431c
> ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java d7edda1
> ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java PRE-CREATION
> service/src/java/org/apache/hive/service/server/HiveServer2.java fa13783
>
> Diff: https://reviews.apache.org/r/17471/diff/
>
>
> Testing
> -------
>
> Added multi-threaded junit tests.
>
>
> Thanks,
>
> Vikram Dixit Kumaraswamy
>
>