> 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 > >
