mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.
Date Wed, 09 Dec 2015 22:46:32 GMT

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


Are you also planning to update https://github.com/apache/mesos/blob/master/docs/oversubscription.md
?


src/Makefile.am (line 1600)
<https://reviews.apache.org/r/40617/#comment169176>

    Great to see a default QoS controller!



src/Makefile.am (lines 1602 - 1603)
<https://reviews.apache.org/r/40617/#comment169181>

    how come the fixed estimator only has a source file and this one has a header too?



src/Makefile.am (line 1674)
<https://reviews.apache.org/r/40617/#comment169177>

    do we need this? i don't see slave/resource_estimators/fixed.cpp here?



src/slave/qos_controllers/load.hpp (line 41)
<https://reviews.apache.org/r/40617/#comment169178>

    s/below/above/ ?



src/slave/qos_controllers/load.hpp (lines 44 - 45)
<https://reviews.apache.org/r/40617/#comment169179>

    Move comment this above the constructor.



src/slave/qos_controllers/load.cpp (lines 96 - 111)
<https://reviews.apache.org/r/40617/#comment169188>

    you can refactor this to use a boolean "loaded" flag and inline `evictRevocableExecutors()`
    
    also, the log lines are misleading. the controller doesn't evict. just say that the "Xmin
load is higher than the threshold.."



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

    the function name is misleading. this is not evicting executors, it is just setting up
corrections. it is the slave that decides to evict it.
    
    also, i don't think this needs to be a function. why not inline this in `__corrections()`.



src/slave/qos_controllers/load.cpp (lines 126 - 142)
<https://reviews.apache.org/r/40617/#comment169186>

    s/targets/corrections/



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

    no need for this temp variable.



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

    kill new line.



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

    why is this a warning instead of an error? seems like the operator would want to know
if he made a typo in the parameter.



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

    ditto.



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

    s/on/one/



src/tests/oversubscription_tests.cpp (lines 1162 - 1175)
<https://reviews.apache.org/r/40617/#comment169211>

    push this inside the lamba below?



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

    i think our style guide discourages use of "=" in favor of explicit capture.



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

    s/First/Second/



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

    this looks bad. you are deleting `stubLoad` while the load qos controller has access to
it.


- Vinod Kone


On Dec. 8, 2015, 11:16 a.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> -----------------------------------------------------------
> 
> (Updated Dec. 8, 2015, 11:16 a.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
> 
>


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