----------------------------------------------------------- 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 > >