> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote: > > src/slave/qos_controllers/load.hpp, lines 39-45 > > <https://reviews.apache.org/r/40617/diff/5/?file=1155926#file1155926line39> > > > > Awesome comment! Let's make it a doxygen style one.
Hmm, i followed all mesos style guidelines e.g https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md.. And mimic other class' comments.. Not sure how to improve that (: > On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote: > > src/slave/qos_controllers/load.cpp, lines 126-142 > > <https://reviews.apache.org/r/40617/diff/5/?file=1155927#file1155927line126> > > > > This is a bit dense block; mind breaking it up a bit? Done. > On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote: > > src/slave/qos_controllers/load.cpp, line 224 > > <https://reviews.apache.org/r/40617/diff/5/?file=1155927#file1155927line224> > > > > 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. Good point. Let's make it consistent with other Mesos messages. (: > On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote: > > src/tests/oversubscription_tests.cpp, line 81 > > <https://reviews.apache.org/r/40617/diff/5/?file=1155928#file1155928line81> > > > > Why this removal? There was sth there i removed some reviews ago... and accidenlty remove that line too. Fixed. > On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote: > > src/tests/oversubscription_tests.cpp, line 1127 > > <https://reviews.apache.org/r/40617/diff/5/?file=1155928#file1155928line1127> > > > > 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? I think i've found even more clean solution - moved to regular instance and just pust the reference to lambda. (: > On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote: > > src/tests/oversubscription_tests.cpp, lines 1143-1150 > > <https://reviews.apache.org/r/40617/diff/5/?file=1155928#file1155928line1143> > > > > Wonder if we can reduce some of this boiler plate; have you taken a > > look at `createTasks()`? Unfortunately, `createTasks` uses offers to create tasks, and beside that it creates tasks _NOT_ `ResourceUsage::Executor`. I cleaned it and made separate inline function for that. Hope it helped. - Bartek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40617/#review109548 ----------------------------------------------------------- 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 > >
