mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.
Date Wed, 09 Dec 2015 18:32:48 GMT

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


This is looking great, Bartek! One more round and I think we are there.


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

    Awesome comment! Let's make it a doxygen style one.



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

    We usually only add our handles to TODOs. Let's just make them `NOTE: ` :)



src/slave/qos_controllers/load.cpp (lines 54 - 55)
<https://reviews.apache.org/r/40617/#comment169118>

    Fits on one line



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

    Also, why not inline `__corrections`? It doesn't look like a continuation and is only
called from one call site (above).
    
    '{' needs to go on next line



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

    Expand '5min'



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

    Same here



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

    This is a bit dense block; mind breaking it up a bit?



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

    Let's remove the trailing '.'
    Maybe we can include some guidance on where to set the thresholds (in the module parameters).
No biggie if you don't want that in.



src/tests/oversubscription_tests.cpp 
<https://reviews.apache.org/r/40617/#comment169151>

    Why this removal?



src/tests/oversubscription_tests.cpp (lines 1110 - 1116)
<https://reviews.apache.org/r/40617/#comment169153>

    Awesome comment!



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

    s/will be above/exceeds/



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

    Can we wrap this in a smart pointer? Is there a potential race between the test code and
the load qos controller when changing the values of the stub load, or how do we guarantee
that?



src/tests/oversubscription_tests.cpp (lines 1142 - 1149)
<https://reviews.apache.org/r/40617/#comment169156>

    Wonder if we can reduce some of this boiler plate; have you taken a look at `createTasks()`?


- Niklas Nielsen


On Dec. 8, 2015, 3: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, 3: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