mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bartek Plotka" <bwplo...@gmail.com>
Subject Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.
Date Thu, 10 Dec 2015 17:12:54 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message