Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-07-27 Thread Reuben Kuhnert 

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

(Updated 七月 27, 2016, 5:35 p.m.)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Diff Rebase


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 14f221a 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
72ad86c 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8 
  shims/scheduler/pom.xml 9141c1e 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244d 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-24 Thread Reuben Kuhnert 


> On May 17, 2016, 6:44 p.m., Yongzhi Chen wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 533
> > 
> >
> > This if statement is duplicate with the Precondition. If you want to 
> > throw exception,only use Precondition, otherwise, just use if statement. 
> > Use both will end up checking the same condition twice.

This is correct. One is for checking that we're in a valid state. The other is 
for throwing if the user tries to call the function in an invalid state. Thanks!


- Reuben


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


On May 24, 2016, 11:56 a.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 24, 2016, 11:56 a.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> 3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
>   ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> a0015ebc655931f241b28c53fbb94cfe172841b1 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
> 63803b8b0752745bd2fedaccc5d100befd97093b 
>   shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
>  372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47040/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Reuben Kuhnert
> 
>

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-24 Thread Reuben Kuhnert 

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

(Updated May 24, 2016, 11:56 a.m.)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Updates to fix test failures.


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-17 Thread Yongzhi Chen 

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




ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 533)


This if statement is duplicate with the Precondition. If you want to throw 
exception,only use Precondition, otherwise, just use if statement. Use both 
will end up checking the same condition twice.


- Yongzhi Chen


On May 14, 2016, 5:51 p.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 14, 2016, 5:51 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> 3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
>   ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> a0015ebc655931f241b28c53fbb94cfe172841b1 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
> 63803b8b0752745bd2fedaccc5d100befd97093b 
>   shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
>  372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47040/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Reuben Kuhnert
> 
>

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-14 Thread Reuben Kuhnert 

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

(Updated May 14, 2016, 5:51 p.m.)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-14 Thread Reuben Kuhnert 

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

(Updated May 14, 2016, 5:47 p.m.)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Remove stringutils changes.


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-14 Thread Reuben Kuhnert 

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

(Updated May 14, 2016, 5:42 p.m.)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Updated diff to include per submission checks and watching changes in 
fair-scheduler.xml location.


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
6d28396893532302fbbd66eace53ae32b71848c3 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairSchedulerQueueAllocator.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-13 Thread Lenni Kuff 

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




shims/common/src/main/java/org/apache/hadoop/fs/FileWatchService.java (line 135)


Add a catch (Exception) so the executor doesn't die if there is an 
unchecked exception thrown for some reason.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 48)


Do we really need to cache this information? Comment on key/value for map.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 51)


Why do we need to track the last used location? Can't we just read the 
location once and use the the whole time?

nit: don't need to initialize to null


- Lenni Kuff


On May 13, 2016, 3:26 p.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 13, 2016, 3:26 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   shims/common/src/main/java/org/apache/hadoop/fs/FileWatchService.java 
> PRE-CREATION 
>   shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
>  372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47040/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Reuben Kuhnert
> 
>

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-13 Thread Reuben Kuhnert 

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

(Updated May 13, 2016, 3:26 p.m.)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  shims/common/src/main/java/org/apache/hadoop/fs/FileWatchService.java 
PRE-CREATION 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-12 Thread Lenni Kuff 

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




common/src/java/org/apache/hive/common/util/HiveStringUtils.java (line 323)


Guava has Strings.isNullOrEmpty(). IF there is already a dependency on 
guava, just use that.



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 58)


It's unclear what validation is actually happening here.



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 65)


Would we ever expect a user to hit this case? If not, it should be 
converted to an invarient / precondition check.



service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java (line 
126)


We should continue to use the flag hive.server2.map.fair.scheduler.queue to 
determine whether or not to enable the mapping functionality



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 42)


You might consider simplifying this a bit since we:

a) don't care about watching multiple files. this should only support 
watching a single file. Can always extend later. 
b) don't care about multiple callbacks. Can always extend later. 
c) don't  care about specific events

For reference, a simplified version is in Impala. You might want to 
consider a similar approach:
I think this can be simplified a bit. Impala did something similar, you 
might consider using a common approach:


https://github.com/cloudera/Impala/blob/cdh5-trunk/fe/src/main/java/com/cloudera/impala/util/FileWatchService.java



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 135)


I don't think we need to support the case where the config file location 
has changed. Hive doesn't dynamically refresh the configs so I'm not sure we 
would see this. For now lets' keep this scoped to only detecting changes to the 
underlying file and using the same path for the course of the operation/


- Lenni Kuff


On May 12, 2016, 1:16 p.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 12, 2016, 1:16 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 6d28396893532302fbbd66eace53ae32b71848c3 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> 3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
>   ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> a0015ebc655931f241b28c53fbb94cfe172841b1 
>   shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
> PRE-CREATION 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
> 63803b8b0752745bd2fedaccc5d100befd97093b 
>   shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
>  372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47040/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Reuben Kuhnert
> 
>

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-12 Thread Reuben Kuhnert 

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

(Updated 5 12, 2016, 1:16 오후)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Update diff


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
6d28396893532302fbbd66eace53ae32b71848c3 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
PRE-CREATION 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-12 Thread Reuben Kuhnert 


> On May 12, 2016, 7:02 a.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 532
> > 
> >
> > Let's rename this to setJobQueueForUser()
> > 
> > Any reason to return the config ? Return value should be void.
> > 
> > configuration -> conf for consistency

Yeah, what you're saying makes sense. The intent here is that I really do want 
to consolidate the scheduling code that's floating around the codebase. Right 
now all this only handles a very specific behavior (sets the job queue if Yarn 
fair scheduling is configured and impersonation is off) - it doesn't even 
handle every Yarn scheduling case, but it really should. This is just me 
setting down a flag and saying "let's consolidate pre-sumbit job scheduler 
stuff here".


> On May 12, 2016, 7:02 a.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java, line 
> > 50
> > 
> >
> > Remove this check.
> > 
> > I'd just inline the setJobQueueForUser method in the caller and get rid 
> > of this entire class altogether. It only seems to be offering 
> > usingNonImpersonationModeWithFairScheduling() method which is a utility 
> > method.

For this issue yes. But we should consolidate all yarn specific behavior in one 
place with a straighforward interface for clients. The alternative is to 
duplicate this:

// In non-impersonation mode, map scheduler queue to current user
// if fair scheduler is configured.
if (! sessionConf.getBoolVar(ConfVars.HIVE_SERVER2_ENABLE_DOAS) &&
  sessionConf.getBoolVar(ConfVars.HIVE_SERVER2_MAP_FAIR_SCHEDULER_QUEUE)) {
  ShimLoader.getHadoopShims().refreshDefaultQueue(sessionConf, username);
}

every time we want to set up a user's job queue in non-impersonation mode. 
Furthermore, I'm absolutely certain there's more Yarn logic than just 'schedule 
a queue in non-impersonation mode' floating around the codebase, but it's hard 
to know exactly what because it's not easily identifiable (see above). A single 
interface for configuring yarn stuff would make it much easier to identify and 
enforce correct behavior.


> On May 12, 2016, 7:02 a.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java, line 
> > 64
> > 
> >
> > What validation we are doing in this method ? I'd just call this 
> > setJobQueueForUser or something

You're correct. The validation should be more rigorous/well-defined than it is 
currently. The long and short is that the user's job queue should be validated 
before jobs are submitted, but it turns out that that's a bit more complicated 
than what we have time for at the moment so in the interim we just check to see 
if the most recent set of rules from fair-scheduler.xml allow you to set the 
same queue as you're already using. So validation *really is* different from 
default configuration, it's just that validation is weakly defined at the 
moment.


> On May 12, 2016, 7:02 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, 
> > line 169
> > 
> >
> > can you elaborate on this comment? Which OSs does it not block ?
> > 
> > If it's async in those OSs, is 5 seconds always sufficient...  seems 
> > hacky.

Yeah, this is actually false. I was testing it wrong previously. Thanks for the 
catch.


> On May 12, 2016, 7:02 a.m., Mohit Sabharwal wrote:
> > shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java,
> >  line 119
> > 
> >
> > I'd rename this method setJobQueueForUser()
> > 
> > Because that's what this method is doing, ... i.e. either setting the 
> > queue config by user or setting default queue config.

Same as above.


> On May 12, 2016, 7:02 a.m., Mohit Sabharwal wrote:
> > shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java,
> >  line 120
> > 
> >
> > Is there any advantage of using "defaultString()" when it returns an 
> > empty string vs just letting conf.get() return null -- and doing a null 
> > check instead of isEmpty() ?
> > 
> > The pattern in Hive code is to just check if conf.get() returns null. 
> > Unless there is a advantage in this use case, let's maintain the pattern. 
> > (Just like the queueConfig != null check elsewhere)
> > 
> > Same for other places where defaultString() is used.

Coming from using F# nulls don't make sense to me, heh. Changed though.


> On May 12, 2016, 7:02 a.m., Mohit

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-12 Thread Mohit Sabharwal 

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



Thanks for making the changes! Had few more comments.


ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 532)


Let's rename this to setJobQueueForUser()

Any reason to return the config ? Return value should be void.

configuration -> conf for consistency



ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 537)


It doesn't make sense to log an error for the case you have already 
established is impossible. IOW, would this line ever be executed ?



ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java (line 35)


don't think this import is used anywhere, so let's remove.



ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java (line 433)


remove empty. We can revert all changes to this file.



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 49)


rename to setDefaultJobQueue()



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 50)


Remove this check.

I'd just inline the setJobQueueForUser method in the caller and get rid of 
this entire class altogether. It only seems to be offering 
usingNonImpersonationModeWithFairScheduling() method which is a utility method.



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 51)


There is no point throwing an exception only to catch it and log an error 
in the only caller. Why not return a boolean?



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 54)


What's the difference between refreshDefaultQueue() in HadoopShims and 
SchedulerShim ?

Here we are using HadoopShims and below we are using SchedulerShim. Why is 
that ?

If you need SchedulerShim, then I'd just get rid of the 
refreshDefaultQueue() method and have a single method called 
setJobQueueForUser() and use it for both default and non-default cases...



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 64)


What validation we are doing in this method ? I'd just call this 
setJobQueueForUser or something



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 42)


remove extra line



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 45)


remove extra line



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 58)


remove extra line



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
120)


rename to getWatchablePath()



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
169)


can you elaborate on this comment? Which OSs does it not block ?

If it's async in those OSs, is 5 seconds always sufficient...  seems hacky.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
229)


nit: move throw to new line



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
238)


nit: move throw to new line



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 112)


I'd rename this method setJobQueueForUser()

Because that's what this method is doing, ... i.e. either setting the queue 
config by user or setting default queue config.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 113)


Is there any advantage of using "defaultString()" when it returns an empty 
string vs just letting conf.get() return null -- and doing a null check instead 
of isEmpty() ?

The pattern in Hive code is to just check if conf.get() returns null. 
Unless there is a advantage in this use case, let's maintain the pattern. (Just 
like the queueConfig != null check elsewhere)

Same for other places where defaultString() is used.

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-11 Thread Reuben Kuhnert 

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

(Updated 5 11, 2016, 8:02 오후)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Add file existence check to FileSystemWatcher


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
926f6e883030b5a01d025994bd02c67f0f5a275c 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
PRE-CREATION 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-11 Thread Reuben Kuhnert 

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

(Updated 5 11, 2016, 7:19 오후)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Added Requested Changes


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 3fecc5c 
  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 926f6e8 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015eb 
  shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
PRE-CREATION 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8 
  shims/scheduler/pom.xml b36c123 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244d 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-11 Thread Reuben Kuhnert 


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java, line 502
> > 
> >
> > Looks like you have the same conditional check both here and in the 
> > function you're calling (validateYarnQueue). In affect, we're doing the 
> > same check twice. Remove one ?

This is an assertion. It's exposed externally so callers can check when to 
perform this operation, but if called in an invalid state, we should fail. 
Ideally, an 'Optional' with a callback would be better though. C'est la vie.


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, 
> > line 136
> > 
> >
> > Should this be synchronized at all ? The event is not shared state, 
> > right ? Are we mutating any shared state in this method ?
> > 
> > Also, make it private ?

'this.callbacks' can be modified from another thread if this function isn't 
monitored. Also, it's made protected for testing purposes (though if there's a 
better practice for this, let me know - not hyper familiar with Java).


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java,
> >  line 64
> > 
> >
> > why synchronized ? Didn't see shared share getting modified anywhere...

Yeah, originally the file-system-watcher was static final (to ensure that if 
multiple instances of the shim exist for some reason, the configuration across 
them all is the same - and callbacks aren't fired multiple times). I ultimately 
decided against this though - this is just a legacy of that failed effort.


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, 
> > lines 74-76
> > 
> >
> > This seems strange.
> > 
> > Why are we closing the watchService() and creating a new one here?
> > 
> > Can the existing watchService() watch more files?
> > 
> > Add comments explaining why this is necessary.

Overuse of 'functional'-style (Prefer new immutable objects over modifying 
existing ones, aka "state is evil"). This introduces a suble bug though if the 
file is modified while the watcher is being re-generated. This can't be helped 
if we're removing a watch though (since there's not 'unregister' function).


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, 
> > line 125
> > 
> >
> > Should this be a warning ?
> > 
> > Also, add the exception to the log.
> > 
> > LOG.warn("..." + ex, ex)
> > 
> > This catch block also doesn't have the close() call. Need a finally 
> > block?

A finally would close the service after each iteration. But I modified 
everything else.


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java,
> >  line 125
> > 
> >
> > rename to getConfigForUser(...)
> > 
> > Also, not quiet sure why this needs to be synchronized.

The configuration resolvers are cached. But the cache is cleared if the 
location of 'fair-scheduler.xml' (YARN_SCHEDULER_FILE_PROPERTY) changes, or is 
modified. No good if one thread clears the cache while another is reading it.


- Reuben


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


On May 10, 2016, 10:54 p.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 10, 2016, 10:54 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 926f6e8 
>

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-10 Thread Mohit Sabharwal 

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




ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java (line 501)


"configuration" seems too verbose compared to how it's typically named in 
Hive code. 

conf ?



ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java (line 502)


Looks like you have the same conditional check both here and in the 
function you're calling (validateYarnQueue). In affect, we're doing the same 
check twice. Remove one ?



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 28)


remove "The YarnFairScheduling class is a "



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 31)


Comment belongs in the javadoc for configureDefualtSchedulerQueue method.



ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 32)


same, i'd use conf just to keep readable & consistent with hive code.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (lines 
21 - 22)


most new hive code uses org.slf4j.Logger



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 26)


Instead of wildcard, use actual imports.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 35)


remove "The FileSystemWatcher "



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 37)


remove empty line



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 40)


remove empty line



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 57)


If it's only for testing, make it public and add @VisibleForTesting 
annotation.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (lines 
74 - 76)


This seems strange.

Why are we closing the watchService() and creating a new one here?

Can the existing watchService() watch more files?

Add comments explaining why this is necessary.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
105)


Please convert this to javadoc. Same in other places.

Comments are typically used inside methods by convention.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
107)


do we not need to also close watchService here ?

if (watchService != null) {
   watchService.close()
}



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (lines 
115 - 123)


Does the entire block needs to be synchronized ? Or just 
watchService.take() ?



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
125)


Should this be a warning ?

Also, add the exception to the log.

LOG.warn("..." + ex, ex)

This catch block also doesn't have the close() call. Need a finally block?



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
127)


Should this be a warning ?

Also, add the exception to the log.

LOG.warn("..." + ex, ex)



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
128)


You probably need to add a finally() block and move the this.close() in 
that block.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
132)


Why are we setting this flag here ? It's not synchronized either.



shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 
136)


Should this be synchronized at all ? The event is not shared state, right ? 
Are we mutating any shared state in this method ?

Also, make it private ?

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-10 Thread Reuben Kuhnert 

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

(Updated 5 10, 2016, 10:54 오후)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Updated diff with unit test


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 926f6e8 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015eb 
  shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
PRE-CREATION 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8 
  shims/scheduler/pom.xml b36c123 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244d 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 PRE-CREATION 
  
shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 PRE-CREATION 

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


Testing
---


Thanks,

Reuben Kuhnert

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

2016-05-09 Thread Reuben Kuhnert 

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

(Updated 5 10, 2016, 1:18 오전)


Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.


Changes
---

Make requested changes to patch


Summary (updated)
-

Monitor changes to FairScheduler.xml file and automatically update / validate 
jobs submitted to fair-scheduler


Bugs: HIVE-13696
https://issues.apache.org/jira/browse/HIVE-13696


Repository: hive-git


Description (updated)
---

Ensure that jobs sent to YARN with impersonation off are correctly routed to 
the proper queue based on fair-scheduler.xml. Monitor this file for changes and 
validate that jobs can only be sent to queues authorized for the user.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
926f6e883030b5a01d025994bd02c67f0f5a275c 
  ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
a0015ebc655931f241b28c53fbb94cfe172841b1 
  shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
PRE-CREATION 
  shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
63803b8b0752745bd2fedaccc5d100befd97093b 
  
shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 

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


Testing
---


Thanks,

Reuben Kuhnert