-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109842
-----------------------------------------------------------



src/slave/qos_controllers/load.cpp (line 76)
<https://reviews.apache.org/r/40617/#comment169479>

    s/Cannot/Failed to/



src/slave/qos_controllers/load.cpp (line 77)
<https://reviews.apache.org/r/40617/#comment169480>

    kill this.



src/slave/qos_controllers/load.cpp (line 81)
<https://reviews.apache.org/r/40617/#comment169481>

    s/overLoaded/overloaded/



src/slave/qos_controllers/load.cpp (line 102)
<https://reviews.apache.org/r/40617/#comment169482>

    we prefer colons for internal protobufs
    
    s/ResourceUSage_Executor/ResourceUsage::Executor/



src/slave/qos_controllers/load.cpp (lines 103 - 106)
<https://reviews.apache.org/r/40617/#comment169483>

    I think you can just say "Set kill correction for all revocable executors." 
at the top instead of these 2 comments.



src/slave/qos_controllers/load.cpp (line 126)
<https://reviews.apache.org/r/40617/#comment169484>

    why protected instead of private?



src/slave/qos_controllers/load.cpp (line 190)
<https://reviews.apache.org/r/40617/#comment169486>

    LOG(ERROR) << "Failed to parse 5min load threshold: " << 
thresholdParam.error();
    return NULL;



src/slave/qos_controllers/load.cpp (line 191)
<https://reviews.apache.org/r/40617/#comment169485>

    just return NULL here. i think we should error out even if one of the 
thresholds is wrong.



src/slave/qos_controllers/load.cpp (line 199)
<https://reviews.apache.org/r/40617/#comment169487>

    ditto. see above.



src/slave/qos_controllers/load.cpp (lines 205 - 210)
<https://reviews.apache.org/r/40617/#comment169488>

    if you do `return`s above, this could just be else block
    
    ```
    } else {
      LOG(ERROR) << "No load thresholds are configured for LoadQoSController";
      return NULL;
    }
    
    ```



src/tests/oversubscription_tests.cpp (line 161)
<https://reviews.apache.org/r/40617/#comment169491>

    it's hard to tell what this method is doing.
    
    i would rather make this a function that returns ExecutorInfo.
    
    ```
    ExecutorInfo createExecutorInfo(
        const string& executorId,
        const string& frameworkId
    )
    ```
    
    do you really need these executors to have different `source`s?



src/tests/oversubscription_tests.cpp (line 1136)
<https://reviews.apache.org/r/40617/#comment169490>

    If "the" total system load on "the" agent exceeds "the" configured 
threshold...



src/tests/oversubscription_tests.cpp (lines 1150 - 1152)
<https://reviews.apache.org/r/40617/#comment169492>

    // Prepare stubbed `os::Load` whose values are below the thresholds.


- Vinod Kone


On Dec. 10, 2015, 5:03 p.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
>     https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added Load QoS Controller for the simple eviction when system load is above 
> configured system load threshold for 5min and 15min:
> - Made os::loadavg called from the lambda and passed via the contructor.
> - Added unit test.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
>   src/slave/qos_controllers/load.hpp PRE-CREATION 
>   src/slave/qos_controllers/load.cpp PRE-CREATION 
>   src/tests/oversubscription_tests.cpp 
> 0333281c247dd182860a49f39be791c00679bf6b 
> 
> Diff: https://reviews.apache.org/r/40617/diff/
> 
> 
> Testing
> -------
> 
> make check
> run mesos with org_apache_mesos_LoadQoSController module included.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Reply via email to