This is an automatically generated e-mail. To reply, visit:

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 

src/Makefile.am (line 1674)

    Tab seems off - set tabstop to 8 :)

src/slave/qos_controllers/load.hpp (line 38)

    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)

    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)

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

src/slave/qos_controllers/load.cpp (line 81)

    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)

    Same as above.

src/slave/qos_controllers/load.cpp (line 106)

    Kill 'this->'

src/slave/qos_controllers/load.cpp (line 115)

    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)

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

src/slave/qos_controllers/load.cpp (line 120)

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

src/slave/qos_controllers/load.cpp (line 125)

    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)

    4 space indent per argument wrapping.

src/slave/qos_controllers/load.cpp (line 168)

    Two newlines between implementing functions here and rest of file

src/slave/qos_controllers/load.cpp (lines 176 - 177)

    4 space indent on argument list wrapping :)

src/slave/qos_controllers/load.cpp (lines 181 - 184)

    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)

    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)

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

src/slave/qos_controllers/load.cpp (lines 214 - 215)

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

src/slave/qos_controllers/load.cpp (line 230)

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

src/tests/oversubscription_tests.cpp (line 63)

    kill this new line

src/tests/oversubscription_tests.cpp (line 171)

    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)

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

src/tests/oversubscription_tests.cpp (line 1157)

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

src/tests/oversubscription_tests.cpp (line 1170)

    Doesn't this need to be indented?

src/tests/oversubscription_tests.cpp (line 1172)

    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

Reply via email to