Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============6238372603981754190=="
MIME-Version: 1.0
Subject: Re: Review Request 62706: HIVE-17473 implement workload management
 pools
From: Sergey Shelukhin <sergey@hortonworks.com>
To: Zhiyuan Yang <sjtu.yzy@gmail.com>, j.prasanth.j@gmail.com
Cc: Sergey Shelukhin <sergey@hortonworks.com>, hive <dev@hive.apache.org>
Date: Wed, 18 Oct 2017 20:44:43 -0000
Message-ID: <20171018204443.63763.47940@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Sergey Shelukhin <noreply@reviews.apache.org>
References: <20171017224836.5693.92676@reviews-vm2.apache.org>
In-Reply-To: <20171017224836.5693.92676@reviews-vm2.apache.org>
Reply-To: Sergey Shelukhin <sergey@hortonworks.com>
archived-at: Wed, 18 Oct 2017 20:44:49 -0000

--===============6238372603981754190==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Oct. 17, 2017, 10:48 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/62706/diff/1/?file=1840587#file1840587line70>
> >
> >     nit: throw if defaultPoolName is still null here.

Why? I actually wonder how this is going to work w.r.t. Tez. I think default pool name might just mean Tez.


> On Oct. 17, 2017, 10:48 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 137 (patched)
> > <https://reviews.apache.org/r/62706/diff/1/?file=1840588#file1840588line141>
> >
> >     should we do some validation here? To check for max depth of tree. May be we shouldn't allow >2 depth. Should we also add limits to how wide the tree can be. Or max children (cannot be greater than some fraction of executor count).

Validation will be performed in a separate jira on apply command, see comments


> On Oct. 17, 2017, 10:48 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Line 126 (original), 225 (patched)
> > <https://reviews.apache.org/r/62706/diff/1/?file=1840588#file1840588line230>
> >
> >     does this launch all sessions synchronously? should we make sure the created sessions are open and are not in accepted state?

This only adds sessions objects. I actually refactored this in the next patch to use a factory instead of passing them in.
The sessions are started async, see TezSessionPool


> On Oct. 17, 2017, 10:48 p.m., Prasanth_J wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java
> > Lines 329 (patched)
> > <https://reviews.apache.org/r/62706/diff/1/?file=1840588#file1840588line338>
> >
> >     isn't this equivalent to sessionsClaimed.acquier()? blocked until acquire

No, we also want to check internalVersion periodically, in case if new plan . Although in the next patch it will be gone anyway.


- Sergey


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


On Sept. 30, 2017, 12:57 a.m., Sergey Shelukhin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62706/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2017, 12:57 a.m.)
> 
> 
> Review request for hive, Zhiyuan Yang and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPool.java 4f2997b95b 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/UserPoolMapping.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/WorkloadManager.java 3f621271cc 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestWorkloadManager.java 7adf895077 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 5cb973ca95 
> 
> 
> Diff: https://reviews.apache.org/r/62706/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>


--===============6238372603981754190==--