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 Sat, 05 Dec 2015 01:11:10 GMT

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


looks great so far! We probably need a few tests to exercise the different scenarios (making
sure that combinations of the thresholds exhibits the right behavior)


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

    Tab seems off - set tabstop to 8 :)



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

    We usually limit the use of typedefs for simpler types (like a list of a concrete type).
Let's expand the use here.



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

    Let's add the QoS controller policy documentation here :) For example, why 5 and 15 minute
thresholds, but not 1 minute?



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

    You don't have to use 'this->' for disambiguation here.



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

    How will this behave? You return a future that will never be satisfied, no? Can you return
the error instead?



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

    Same as above.



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

    Kill 'this->'



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

    We usually don't know 'getXYZ', if you can infer the operation type from the name. For
example, just calling it 'loadAvg()', which in fact should be 'loadAverage()' to be pedantic
:)



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

    You evict everything by issuing corrections, what do you think about using that term?



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

    How about calling them 'preemptionTargets' or 'evictionTargets' or simply 'targets'?



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

    Not only the executors disappear, the entire container is nuked. Maybe we should refer
to executor+tasks or just, 'container'.



src/slave/qos_controllers/load.cpp (lines 157 - 162)
<https://reviews.apache.org/r/40617/#comment168465>

    4 space indent per argument wrapping.



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

    Two newlines between implementing functions here and rest of file



src/slave/qos_controllers/load.cpp (lines 176 - 177)
<https://reviews.apache.org/r/40617/#comment168462>

    4 space indent on argument list wrapping :)



src/slave/qos_controllers/load.cpp (lines 181 - 184)
<https://reviews.apache.org/r/40617/#comment168461>

    We usually limit this kind of single call wrappers. Doesn't change the abstraction/return
type of os::loadavg().
    
    I see it is for the test; can we create a c++11 lambda in place with `[](){return os::load();}`
instead?
    
    I'd like to get rid of the wrapper - it's a bit of a code smell :)



src/slave/qos_controllers/load.cpp (lines 190 - 195)
<https://reviews.apache.org/r/40617/#comment168484>

    We can skip this for now and leave a NULL in the compatibility function pointer field.



src/slave/qos_controllers/load.cpp (lines 204 - 206)
<https://reviews.apache.org/r/40617/#comment168482>

    stout has string parsing, take a look at `numify<>()` :)



src/slave/qos_controllers/load.cpp (lines 214 - 215)
<https://reviews.apache.org/r/40617/#comment168481>

    Move the else line up, so it becomes: 
    
    ```
    } else if (...
    ```



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

    Should we throw a warning or error here instead of just a NULL pointer?



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

    kill this new line



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

    Think we should refer to the load function with ``'s. Try to see Greg Mann's proposal
on the dev list :)



src/tests/oversubscription_tests.cpp (lines 199 - 201)
<https://reviews.apache.org/r/40617/#comment168487>

    We need to do a scan for argument wrapping. Should be 4 space indent.



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

    Instead of refering to BE, we standardized on 'recovable' I think. @Vinod: what do you
think?



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

    Doesn't this need to be indented?



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

    We should probably add some documentation on where these numbers come from


- Niklas Nielsen


On Nov. 26, 2015, 4:05 a.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2015, 4:05 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added Load QoS Controller for the simple eviction when system load is above configured
threshold:
> - Made os::loadavg called from internal method. This method is then passed as lambda
to the Load QoS Controller process.
> - Added MockLoadQoSController
> - Added tests
> 
> Tests still WIP.
> 
> 
> 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
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>


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