----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40617/#review109842 -----------------------------------------------------------
src/slave/qos_controllers/load.cpp (line 76) <https://reviews.apache.org/r/40617/#comment169479> s/Cannot/Failed to/ src/slave/qos_controllers/load.cpp (line 77) <https://reviews.apache.org/r/40617/#comment169480> kill this. src/slave/qos_controllers/load.cpp (line 81) <https://reviews.apache.org/r/40617/#comment169481> s/overLoaded/overloaded/ src/slave/qos_controllers/load.cpp (line 102) <https://reviews.apache.org/r/40617/#comment169482> we prefer colons for internal protobufs s/ResourceUSage_Executor/ResourceUsage::Executor/ src/slave/qos_controllers/load.cpp (lines 103 - 106) <https://reviews.apache.org/r/40617/#comment169483> I think you can just say "Set kill correction for all revocable executors." at the top instead of these 2 comments. src/slave/qos_controllers/load.cpp (line 126) <https://reviews.apache.org/r/40617/#comment169484> why protected instead of private? src/slave/qos_controllers/load.cpp (line 190) <https://reviews.apache.org/r/40617/#comment169486> LOG(ERROR) << "Failed to parse 5min load threshold: " << thresholdParam.error(); return NULL; src/slave/qos_controllers/load.cpp (line 191) <https://reviews.apache.org/r/40617/#comment169485> just return NULL here. i think we should error out even if one of the thresholds is wrong. src/slave/qos_controllers/load.cpp (line 199) <https://reviews.apache.org/r/40617/#comment169487> ditto. see above. src/slave/qos_controllers/load.cpp (lines 205 - 210) <https://reviews.apache.org/r/40617/#comment169488> if you do `return`s above, this could just be else block ``` } else { LOG(ERROR) << "No load thresholds are configured for LoadQoSController"; return NULL; } ``` src/tests/oversubscription_tests.cpp (line 161) <https://reviews.apache.org/r/40617/#comment169491> it's hard to tell what this method is doing. i would rather make this a function that returns ExecutorInfo. ``` ExecutorInfo createExecutorInfo( const string& executorId, const string& frameworkId ) ``` do you really need these executors to have different `source`s? src/tests/oversubscription_tests.cpp (line 1136) <https://reviews.apache.org/r/40617/#comment169490> If "the" total system load on "the" agent exceeds "the" configured threshold... src/tests/oversubscription_tests.cpp (lines 1150 - 1152) <https://reviews.apache.org/r/40617/#comment169492> // Prepare stubbed `os::Load` whose values are below the thresholds. - Vinod Kone On Dec. 10, 2015, 5:03 p.m., Bartek Plotka wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40617/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2015, 5:03 p.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 > >
