You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/12/05 02:11:10 UTC
Re: Review Request 40617: Added Load QoS Controller for simple
eviction when system load is above configured threshold.
-----------------------------------------------------------
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
>
>