----------------------------------------------------------- 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) <https://reviews.apache.org/r/47040/#comment197072> 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) <https://reviews.apache.org/r/47040/#comment197097> 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) <https://reviews.apache.org/r/47040/#comment197070> 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) <https://reviews.apache.org/r/47040/#comment197071> remove empty. We can revert all changes to this file. ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 49) <https://reviews.apache.org/r/47040/#comment197093> rename to setDefaultJobQueue() ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java (line 50) <https://reviews.apache.org/r/47040/#comment197096> 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) <https://reviews.apache.org/r/47040/#comment197095> 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) <https://reviews.apache.org/r/47040/#comment197094> 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) <https://reviews.apache.org/r/47040/#comment197092> 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) <https://reviews.apache.org/r/47040/#comment197098> remove extra line shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 45) <https://reviews.apache.org/r/47040/#comment197099> remove extra line shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 58) <https://reviews.apache.org/r/47040/#comment197100> remove extra line shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 120) <https://reviews.apache.org/r/47040/#comment197101> rename to getWatchablePath() shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 169) <https://reviews.apache.org/r/47040/#comment197102> 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) <https://reviews.apache.org/r/47040/#comment197103> nit: move throw to new line shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java (line 238) <https://reviews.apache.org/r/47040/#comment197104> nit: move throw to new line shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 112) <https://reviews.apache.org/r/47040/#comment197090> 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) <https://reviews.apache.org/r/47040/#comment197081> 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. shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 117) <https://reviews.apache.org/r/47040/#comment197089> wait, refreshDefaultQueue() just calls attemptSetScheduleForUser(). Can you just inline attemptSetScheduleForUser here with YarnConfiguration.DEFAULT_QUEUE_NAME param ? It's cleaner without the extra indirection. shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 121) <https://reviews.apache.org/r/47040/#comment197087> Call this setJobQueueForUserInternal ? attemptSetScheduleForUser seems confusing to me. I thought we're just setting job queues and Yarn does the scheduling... configuration -> conf shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 124) <https://reviews.apache.org/r/47040/#comment197085> remove extra line shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 125) <https://reviews.apache.org/r/47040/#comment197086> nit: cleaner with if (queueConfig == null) return; shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 130) <https://reviews.apache.org/r/47040/#comment197091> We should make this LOG.info. There is nothing warn about here ... shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java (line 137) <https://reviews.apache.org/r/47040/#comment197079> config for consistency shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java (line 1) <https://reviews.apache.org/r/47040/#comment197105> Add Apache License. - Mohit Sabharwal On May 11, 2016, 8:02 p.m., Reuben Kuhnert wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47040/ > ----------------------------------------------------------- > > (Updated May 11, 2016, 8:02 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/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 > >
