----------------------------------------------------------- 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) <https://reviews.apache.org/r/47040/#comment196788> "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) <https://reviews.apache.org/r/47040/#comment196825> 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) <https://reviews.apache.org/r/47040/#comment196830> remove "The YarnFairScheduling class is a " ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 31) <https://reviews.apache.org/r/47040/#comment196823> Comment belongs in the javadoc for configureDefualtSchedulerQueue method. ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 32) <https://reviews.apache.org/r/47040/#comment196824> 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) <https://reviews.apache.org/r/47040/#comment196827> most new hive code uses org.slf4j.Logger shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 26) <https://reviews.apache.org/r/47040/#comment196829> Instead of wildcard, use actual imports. shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 35) <https://reviews.apache.org/r/47040/#comment196831> remove "The FileSystemWatcher " shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 37) <https://reviews.apache.org/r/47040/#comment196826> remove empty line shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 40) <https://reviews.apache.org/r/47040/#comment196828> remove empty line shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 57) <https://reviews.apache.org/r/47040/#comment196832> 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) <https://reviews.apache.org/r/47040/#comment196836> 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) <https://reviews.apache.org/r/47040/#comment196837> 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) <https://reviews.apache.org/r/47040/#comment196835> 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) <https://reviews.apache.org/r/47040/#comment196838> 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) <https://reviews.apache.org/r/47040/#comment196839> 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) <https://reviews.apache.org/r/47040/#comment196840> 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) <https://reviews.apache.org/r/47040/#comment196841> 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) <https://reviews.apache.org/r/47040/#comment196842> 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) <https://reviews.apache.org/r/47040/#comment196843> 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 ? shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java (line 1) <https://reviews.apache.org/r/47040/#comment196844> Missing Apache License Header shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java (line 11) <https://reviews.apache.org/r/47040/#comment196845> Remove and add javadoc relevant to the implementation. shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java (line 29) <https://reviews.apache.org/r/47040/#comment196846> throw new RuntimeException The exception could have been thrown for any unknown reason. shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java (line 41) <https://reviews.apache.org/r/47040/#comment196847> remove empty line. shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 50) <https://reviews.apache.org/r/47040/#comment196850> why synchronized ? shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 53) <https://reviews.apache.org/r/47040/#comment196849> Change it to (Exception ex) shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 55) <https://reviews.apache.org/r/47040/#comment196848> Log exception in addition to the message. i.e. LOG.error(messge, ex) shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 60) <https://reviews.apache.org/r/47040/#comment196851> why synchronized ? Didn't see shared share getting modified anywhere... shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 114) <https://reviews.apache.org/r/47040/#comment196852> Important enough that this should be INFO level ? shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 121) <https://reviews.apache.org/r/47040/#comment196853> rename to getConfigForUser(...) Also, not quiet sure why this needs to be synchronized. shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 134) <https://reviews.apache.org/r/47040/#comment196854> Instead of String.format, you can use slf4j.Logger format ("{}"). Log the exception as well, i.e. Log.warn(msg, ex) shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 143) <https://reviews.apache.org/r/47040/#comment196855> Instead of String.format, you can use slf4j.Logger format ("{}"). Log the exception as well, i.e. Log.warn(msg, ex) shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 163) <https://reviews.apache.org/r/47040/#comment196856> convert to javadoc shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java (line 1) <https://reviews.apache.org/r/47040/#comment196857> Missing Apache License Header shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java (line 10) <https://reviews.apache.org/r/47040/#comment196858> Remove and add relevant javadoc shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java (line 1) <https://reviews.apache.org/r/47040/#comment196859> Missing Apache header - Mohit Sabharwal 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 > >
