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