hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mohit Sabharwal <mo...@cloudera.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 02:23:07 GMT

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


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