You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2014/10/28 18:48:31 UTC
Review Request 27292: HIVE-8634: HiveServer2 fair scheduler queue
mapping doesn't handle the secondary groups rules correctly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27292/
-----------------------------------------------------------
Review request for hive, Brock Noland and Mohit Sabharwal.
Bugs: HIVE-8634
https://issues.apache.org/jira/browse/HIVE-8634
Repository: hive-git
Description
-------
The fair scheduler queue refresh in HiveServer2 (for non-impersonation mode), doesn't handle the primary/secondary queue mappings correctly. It's not reading primary and secondary rules from the scheduler rule file.
The patch involves refreshing the queue policy allocation to read the fair-scheduler.xml file correctly.
Diffs
-----
data/conf/fair-scheduler-test.xml PRE-CREATION
itests/hive-unit-hadoop2/src/test/java/org/apache/hive/jdbc/TestSchedulerQueue.java 79878ba
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 6125714
Diff: https://reviews.apache.org/r/27292/diff/
Testing
-------
Added test cases to for various queue allocation rules.
Thanks,
Prasad Mujumdar
Re: Review Request 27292: HIVE-8634: HiveServer2 fair scheduler queue
mapping doesn't handle the secondary groups rules correctly
Posted by Mohit Sabharwal <mo...@cloudera.com>.
> On Oct. 29, 2014, 1:39 a.m., Prasad Mujumdar wrote:
> > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java, line 237
> > <https://reviews.apache.org/r/27292/diff/1/?file=735526#file735526line237>
> >
> > - That's correct, we'll reload the config for each new session.
> > - Calling setReloadListener() is the only way to get back the full AllocationConfiguration (including the yarn config and fair-scheduler.xml). Without the callback, we can't access those details from the calling method. Does that answer the question ?
> >
> > - yes, we don't really need to protect the config. The AtomicReference it just a workaround to retriev the refernced object to a nested block
Thanks! LGTM.
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27292/#review58948
-----------------------------------------------------------
On Oct. 28, 2014, 5:48 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27292/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 5:48 p.m.)
>
>
> Review request for hive, Brock Noland and Mohit Sabharwal.
>
>
> Bugs: HIVE-8634
> https://issues.apache.org/jira/browse/HIVE-8634
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The fair scheduler queue refresh in HiveServer2 (for non-impersonation mode), doesn't handle the primary/secondary queue mappings correctly. It's not reading primary and secondary rules from the scheduler rule file.
> The patch involves refreshing the queue policy allocation to read the fair-scheduler.xml file correctly.
>
>
> Diffs
> -----
>
> data/conf/fair-scheduler-test.xml PRE-CREATION
> itests/hive-unit-hadoop2/src/test/java/org/apache/hive/jdbc/TestSchedulerQueue.java 79878ba
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 6125714
>
> Diff: https://reviews.apache.org/r/27292/diff/
>
>
> Testing
> -------
>
> Added test cases to for various queue allocation rules.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 27292: HIVE-8634: HiveServer2 fair scheduler queue
mapping doesn't handle the secondary groups rules correctly
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27292/#review58948
-----------------------------------------------------------
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/27292/#comment100156>
- That's correct, we'll reload the config for each new session.
- Calling setReloadListener() is the only way to get back the full AllocationConfiguration (including the yarn config and fair-scheduler.xml). Without the callback, we can't access those details from the calling method. Does that answer the question ?
- yes, we don't really need to protect the config. The AtomicReference it just a workaround to retriev the refernced object to a nested block
- Prasad Mujumdar
On Oct. 28, 2014, 5:48 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27292/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 5:48 p.m.)
>
>
> Review request for hive, Brock Noland and Mohit Sabharwal.
>
>
> Bugs: HIVE-8634
> https://issues.apache.org/jira/browse/HIVE-8634
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The fair scheduler queue refresh in HiveServer2 (for non-impersonation mode), doesn't handle the primary/secondary queue mappings correctly. It's not reading primary and secondary rules from the scheduler rule file.
> The patch involves refreshing the queue policy allocation to read the fair-scheduler.xml file correctly.
>
>
> Diffs
> -----
>
> data/conf/fair-scheduler-test.xml PRE-CREATION
> itests/hive-unit-hadoop2/src/test/java/org/apache/hive/jdbc/TestSchedulerQueue.java 79878ba
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 6125714
>
> Diff: https://reviews.apache.org/r/27292/diff/
>
>
> Testing
> -------
>
> Added test cases to for various queue allocation rules.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 27292: HIVE-8634: HiveServer2 fair scheduler queue
mapping doesn't handle the secondary groups rules correctly
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27292/#review59023
-----------------------------------------------------------
Ship it!
LGTM! Thank you Prasad! Some very minor items below, please update and then submit a new patch. I will +1 on JIRA.
data/conf/fair-scheduler-test.xml
<https://reviews.apache.org/r/27292/#comment100297>
trim trailing ws
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/27292/#comment100299>
Yeah let's make:
throw new IOException("Failed to load queue allocations", ex);
this:
throw new IOException("Failed to load queue allocations file: " + ex, ex);
the + ex gives the user (1) the name of the exception and (2) message all in the first error they see
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/27292/#comment100298>
Based on the code, I don't think this should occur. If it does, should we put out a error message?
- Brock Noland
On Oct. 28, 2014, 5:48 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27292/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 5:48 p.m.)
>
>
> Review request for hive, Brock Noland and Mohit Sabharwal.
>
>
> Bugs: HIVE-8634
> https://issues.apache.org/jira/browse/HIVE-8634
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The fair scheduler queue refresh in HiveServer2 (for non-impersonation mode), doesn't handle the primary/secondary queue mappings correctly. It's not reading primary and secondary rules from the scheduler rule file.
> The patch involves refreshing the queue policy allocation to read the fair-scheduler.xml file correctly.
>
>
> Diffs
> -----
>
> data/conf/fair-scheduler-test.xml PRE-CREATION
> itests/hive-unit-hadoop2/src/test/java/org/apache/hive/jdbc/TestSchedulerQueue.java 79878ba
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 6125714
>
> Diff: https://reviews.apache.org/r/27292/diff/
>
>
> Testing
> -------
>
> Added test cases to for various queue allocation rules.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 27292: HIVE-8634: HiveServer2 fair scheduler queue
mapping doesn't handle the secondary groups rules correctly
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27292/#review58872
-----------------------------------------------------------
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/27292/#comment100030>
just so i understand this: we will reload the config every time this method is called (from new session), right ?
should we move the setReloadListner() call before allocsLoader.init() because reloadAllocations() is also called inside init(). In which case, we don't need to explicitly call allocsLoader.reloadAllocations(), right ?
Also, do we really need to protect the config with AtomicReference ?
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/27292/#comment100024>
nit: "Failed to load queue allocations file"
- Mohit Sabharwal
On Oct. 28, 2014, 5:48 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27292/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 5:48 p.m.)
>
>
> Review request for hive, Brock Noland and Mohit Sabharwal.
>
>
> Bugs: HIVE-8634
> https://issues.apache.org/jira/browse/HIVE-8634
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> The fair scheduler queue refresh in HiveServer2 (for non-impersonation mode), doesn't handle the primary/secondary queue mappings correctly. It's not reading primary and secondary rules from the scheduler rule file.
> The patch involves refreshing the queue policy allocation to read the fair-scheduler.xml file correctly.
>
>
> Diffs
> -----
>
> data/conf/fair-scheduler-test.xml PRE-CREATION
> itests/hive-unit-hadoop2/src/test/java/org/apache/hive/jdbc/TestSchedulerQueue.java 79878ba
> shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 6125714
>
> Diff: https://reviews.apache.org/r/27292/diff/
>
>
> Testing
> -------
>
> Added test cases to for various queue allocation rules.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>