hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Reuben Kuhnert <sircodesa...@gmail.com>
Subject Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler
Date Wed, 11 May 2016 19:17:32 GMT


> 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
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378910#file1378910line502>
> >
> >     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
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378913#file1378913line136>
> >
> >     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
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378917#file1378917line64>
> >
> >     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
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378913#file1378913line74>
> >
> >     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
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378913#file1378913line125>
> >
> >     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
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378917#file1378917line125>
> >
> >     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 
>   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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message