----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40617/#review109597 -----------------------------------------------------------
Are you also planning to update https://github.com/apache/mesos/blob/master/docs/oversubscription.md ? src/Makefile.am (line 1600) <https://reviews.apache.org/r/40617/#comment169176> Great to see a default QoS controller! src/Makefile.am (lines 1602 - 1603) <https://reviews.apache.org/r/40617/#comment169181> how come the fixed estimator only has a source file and this one has a header too? src/Makefile.am (line 1674) <https://reviews.apache.org/r/40617/#comment169177> do we need this? i don't see slave/resource_estimators/fixed.cpp here? src/slave/qos_controllers/load.hpp (line 41) <https://reviews.apache.org/r/40617/#comment169178> s/below/above/ ? src/slave/qos_controllers/load.hpp (lines 44 - 45) <https://reviews.apache.org/r/40617/#comment169179> Move comment this above the constructor. src/slave/qos_controllers/load.cpp (lines 96 - 111) <https://reviews.apache.org/r/40617/#comment169188> you can refactor this to use a boolean "loaded" flag and inline `evictRevocableExecutors()` also, the log lines are misleading. the controller doesn't evict. just say that the "Xmin load is higher than the threshold.." src/slave/qos_controllers/load.cpp (line 124) <https://reviews.apache.org/r/40617/#comment169185> the function name is misleading. this is not evicting executors, it is just setting up corrections. it is the slave that decides to evict it. also, i don't think this needs to be a function. why not inline this in `__corrections()`. src/slave/qos_controllers/load.cpp (lines 126 - 142) <https://reviews.apache.org/r/40617/#comment169186> s/targets/corrections/ src/slave/qos_controllers/load.cpp (line 128) <https://reviews.apache.org/r/40617/#comment169187> no need for this temp variable. src/slave/qos_controllers/load.cpp (line 189) <https://reviews.apache.org/r/40617/#comment169184> kill new line. src/slave/qos_controllers/load.cpp (line 206) <https://reviews.apache.org/r/40617/#comment169182> why is this a warning instead of an error? seems like the operator would want to know if he made a typo in the parameter. src/slave/qos_controllers/load.cpp (line 215) <https://reviews.apache.org/r/40617/#comment169183> ditto. src/tests/oversubscription_tests.cpp (line 1121) <https://reviews.apache.org/r/40617/#comment169207> s/on/one/ src/tests/oversubscription_tests.cpp (lines 1162 - 1175) <https://reviews.apache.org/r/40617/#comment169211> push this inside the lamba below? src/tests/oversubscription_tests.cpp (line 1178) <https://reviews.apache.org/r/40617/#comment169213> i think our style guide discourages use of "=" in favor of explicit capture. src/tests/oversubscription_tests.cpp (line 1188) <https://reviews.apache.org/r/40617/#comment169214> s/First/Second/ src/tests/oversubscription_tests.cpp (line 1196) <https://reviews.apache.org/r/40617/#comment169215> this looks bad. you are deleting `stubLoad` while the load qos controller has access to it. - Vinod Kone On Dec. 8, 2015, 11: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, 11: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 > >
