Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Dec. 14, 2015, 9:47 a.m.)


Review request for mesos, Niklas Nielsen and Vinod Kone.


Changes
---

Addressed last issue with log string.


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 (updated)
-

  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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Dec. 11, 2015, 12:45 p.m.)


Review request for mesos, Niklas Nielsen and Vinod Kone.


Changes
---

Issuess addressed.
- Refactored comments.
- Style issues.
- Moved protected to private
- Changed error handling - if any of thresholds are not parsable then not load 
the module.


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 (updated)
-

  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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 10, 2015, 7:58 p.m., Vinod Kone wrote:
> > src/slave/qos_controllers/load.cpp, lines 236-241
> > 
> >
> > if you do `return`s above, this could just be else block
> > 
> > ```
> > } else {
> >   LOG(ERROR) << "No load thresholds are configured for 
> > LoadQoSController";
> >   return NULL;
> > }
> > 
> > ```

Hmm.. i think we would then miss the case when user configure any other junk 
parameter. I think we want to ignore that and end the end check if any of the 
important parameters (threshold) are set.


> On Dec. 10, 2015, 7:58 p.m., Vinod Kone wrote:
> > src/tests/oversubscription_tests.cpp, line 161
> > 
> >
> > 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?

Nope, it is unnecessary. Killed.


- Bartek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109842
---


On Dec. 11, 2015, 12:45 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 11, 2015, 12:45 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review110017
---

Ship it!



src/slave/qos_controllers/load.cpp (line 206)


s/threshold/load thresholds/


- Vinod Kone


On Dec. 11, 2015, 12:45 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 11, 2015, 12:45 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 1600
> > 
> >
> > Great to see a default QoS controller!

Thx, can i `fix` that? =DDD


- Bartek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109597
---


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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 6:32 p.m., Niklas Nielsen wrote:
> > src/slave/qos_controllers/load.hpp, lines 39-45
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Vinod Kone]

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


s/Cannot/Failed to/



src/slave/qos_controllers/load.cpp (line 77)


kill this.



src/slave/qos_controllers/load.cpp (line 81)


s/overLoaded/overloaded/



src/slave/qos_controllers/load.cpp (line 102)


we prefer colons for internal protobufs

s/ResourceUSage_Executor/ResourceUsage::Executor/



src/slave/qos_controllers/load.cpp (lines 103 - 106)


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)


why protected instead of private?



src/slave/qos_controllers/load.cpp (line 190)


LOG(ERROR) << "Failed to parse 5min load threshold: " << 
thresholdParam.error();
return NULL;



src/slave/qos_controllers/load.cpp (line 191)


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)


ditto. see above.



src/slave/qos_controllers/load.cpp (lines 205 - 210)


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)


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)


If "the" total system load on "the" agent exceeds "the" configured 
threshold...



src/tests/oversubscription_tests.cpp (lines 1150 - 1152)


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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > Are you also planning to update 
> > https://github.com/apache/mesos/blob/master/docs/oversubscription.md ?

Already in review: https://reviews.apache.org/r/41042/  (:


Thx for detailed review i will fix the issues soon.


> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 1602-1603
> > 
> >
> > how come the fixed estimator only has a source file and this one has a 
> > header too?

I needed Load QoS controller interface (in header file) for the unit tests in 
`oversubscription_tests.hpp`. Fixed estimator was tested as integration test. 
(Loaded as module and checking status update message).


> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 1674
> > 
> >
> > do we need this? i don't see slave/resource_estimators/fixed.cpp here?

See comment above. I need that for my unit tests. Fixed estimator has only 
integration test.


- Bartek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109597
---


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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Dec. 9, 2015, 10:46 p.m., Vinod Kone wrote:
> > src/tests/oversubscription_tests.cpp, lines 1163-1176
> > 
> >
> > push this inside the lamba below?

Can i ask why? (: From the computation perspective now i will create such 
structure only once and only copy, whereas in lambda we would create it in 
every lambda call


- Bartek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109597
---


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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109798
---

Ship it!


Looking great! Will let Vinod have one more look, but otherwise looks good to 
go.


src/slave/qos_controllers/load.hpp (line 47)


Minor s/loadAvg/loadAverage/



src/tests/oversubscription_tests.cpp (line 1151)


Newline after


- Niklas Nielsen


On Dec. 10, 2015, 9:03 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 9:03 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

> On Dec. 9, 2015, 10:32 a.m., Niklas Nielsen wrote:
> > src/slave/qos_controllers/load.hpp, lines 39-45
> > 
> >
> > Awesome comment! Let's make it a doxygen style one.
> 
> Bartek Plotka wrote:
> 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 (:

nvm - feel free to drop


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109548
---


On Dec. 10, 2015, 9:03 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 9:03 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

> On Dec. 9, 2015, 2:46 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 1600
> > 
> >
> > Great to see a default QoS controller!
> 
> Bartek Plotka wrote:
> Thx, can i `fix` that? =DDD

@vinod - maybe wasn't meant as an issue?


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109597
---


On Dec. 10, 2015, 9:03 a.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Dec. 10, 2015, 9:03 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

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


Changes
---

Nik & Vinod issues addressed (:


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 (updated)
-

  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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

> On Gru 9, 2015, 10:46 po południu, Vinod Kone wrote:
> > src/Makefile.am, line 1600
> > 
> >
> > Great to see a default QoS controller!
> 
> Bartek Plotka wrote:
> Thx, can i `fix` that? =DDD
> 
> Niklas Nielsen wrote:
> @vinod - maybe wasn't meant as an issue?

i am just kidding, sure it is (:


- Bartek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109597
---


On Gru 10, 2015, 5:03 po południu, Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40617/
> ---
> 
> (Updated Gru 10, 2015, 5:03 po południu)
> 
> 
> 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
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

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


Awesome comment! Let's make it a doxygen style one.



src/slave/qos_controllers/load.hpp (line 42)


We usually only add our handles to TODOs. Let's just make them `NOTE: ` :)



src/slave/qos_controllers/load.cpp (lines 54 - 55)


Fits on one line



src/slave/qos_controllers/load.cpp (line 89)


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)


Expand '5min'



src/slave/qos_controllers/load.cpp (line 108)


Same here



src/slave/qos_controllers/load.cpp (lines 126 - 142)


This is a bit dense block; mind breaking it up a bit?



src/slave/qos_controllers/load.cpp (line 224)


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 


Why this removal?



src/tests/oversubscription_tests.cpp (lines 1110 - 1116)


Awesome comment!



src/tests/oversubscription_tests.cpp (line )


s/will be above/exceeds/



src/tests/oversubscription_tests.cpp (line 1126)


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)


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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Vinod Kone]

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


Great to see a default QoS controller!



src/Makefile.am (lines 1602 - 1603)


how come the fixed estimator only has a source file and this one has a 
header too?



src/Makefile.am (line 1674)


do we need this? i don't see slave/resource_estimators/fixed.cpp here?



src/slave/qos_controllers/load.hpp (line 41)


s/below/above/ ?



src/slave/qos_controllers/load.hpp (lines 44 - 45)


Move comment this above the constructor.



src/slave/qos_controllers/load.cpp (lines 96 - 111)


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)


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)


s/targets/corrections/



src/slave/qos_controllers/load.cpp (line 128)


no need for this temp variable.



src/slave/qos_controllers/load.cpp (line 189)


kill new line.



src/slave/qos_controllers/load.cpp (line 206)


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)


ditto.



src/tests/oversubscription_tests.cpp (line 1121)


s/on/one/



src/tests/oversubscription_tests.cpp (lines 1162 - 1175)


push this inside the lamba below?



src/tests/oversubscription_tests.cpp (line 1178)


i think our style guide discourages use of "=" in favor of explicit capture.



src/tests/oversubscription_tests.cpp (line 1188)


s/First/Second/



src/tests/oversubscription_tests.cpp (line 1196)


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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

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


Changes
---

Moved to standard license header (with `//` comment syntax)


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 (updated)
-

  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

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Dec. 7, 2015, 3:05 p.m.)


Review request for mesos and Niklas Nielsen.


Repository: mesos


Description (updated)
---

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


Thanks,

Bartek Plotka

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Dec. 7, 2015, 3:02 p.m.)


Review request for mesos and Niklas Nielsen.


Changes
---

- Added few descriptions and comments.
- Moved wrapping function to c++11 lambda.
- Removed simple typedef.
- Moved isstring to numify.
- Added proper unit test.


Repository: mesos


Description
---

Added Load QoS Controller for the simple eviction when system load is above 
configured threshold:
- Made os::loadavg called from internal method. This method is then passed as 
lambda to the Load QoS Controller process.
- Added MockLoadQoSController
- Added tests

Tests still WIP.


Diffs (updated)
-

  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


Thanks,

Bartek Plotka

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Dec. 7, 2015, 4:40 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 (updated)
---

make check
run mesos with org_apache_mesos_LoadQoSController module included.


Thanks,

Bartek Plotka

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/#review109057
---


looks great so far! We probably need a few tests to exercise the different 
scenarios (making sure that combinations of the thresholds exhibits the right 
behavior)


src/Makefile.am (line 1674)


Tab seems off - set tabstop to 8 :)



src/slave/qos_controllers/load.hpp (line 38)


We usually limit the use of typedefs for simpler types (like a list of a 
concrete type). Let's expand the use here.



src/slave/qos_controllers/load.hpp (line 40)


Let's add the QoS controller policy documentation here :) For example, why 
5 and 15 minute thresholds, but not 1 minute?



src/slave/qos_controllers/load.cpp (line 77)


You don't have to use 'this->' for disambiguation here.



src/slave/qos_controllers/load.cpp (line 81)


How will this behave? You return a future that will never be satisfied, no? 
Can you return the error instead?



src/slave/qos_controllers/load.cpp (line 85)


Same as above.



src/slave/qos_controllers/load.cpp (line 106)


Kill 'this->'



src/slave/qos_controllers/load.cpp (line 115)


We usually don't know 'getXYZ', if you can infer the operation type from 
the name. For example, just calling it 'loadAvg()', which in fact should be 
'loadAverage()' to be pedantic :)



src/slave/qos_controllers/load.cpp (line 119)


You evict everything by issuing corrections, what do you think about using 
that term?



src/slave/qos_controllers/load.cpp (line 120)


How about calling them 'preemptionTargets' or 'evictionTargets' or simply 
'targets'?



src/slave/qos_controllers/load.cpp (line 125)


Not only the executors disappear, the entire container is nuked. Maybe we 
should refer to executor+tasks or just, 'container'.



src/slave/qos_controllers/load.cpp (lines 157 - 162)


4 space indent per argument wrapping.



src/slave/qos_controllers/load.cpp (line 168)


Two newlines between implementing functions here and rest of file



src/slave/qos_controllers/load.cpp (lines 176 - 177)


4 space indent on argument list wrapping :)



src/slave/qos_controllers/load.cpp (lines 181 - 184)


We usually limit this kind of single call wrappers. Doesn't change the 
abstraction/return type of os::loadavg().

I see it is for the test; can we create a c++11 lambda in place with 
`[](){return os::load();}` instead?

I'd like to get rid of the wrapper - it's a bit of a code smell :)



src/slave/qos_controllers/load.cpp (lines 190 - 195)


We can skip this for now and leave a NULL in the compatibility function 
pointer field.



src/slave/qos_controllers/load.cpp (lines 204 - 206)


stout has string parsing, take a look at `numify<>()` :)



src/slave/qos_controllers/load.cpp (lines 214 - 215)


Move the else line up, so it becomes: 

```
} else if (...
```



src/slave/qos_controllers/load.cpp (line 230)


Should we throw a warning or error here instead of just a NULL pointer?



src/tests/oversubscription_tests.cpp (line 63)


kill this new line



src/tests/oversubscription_tests.cpp (line 171)


Think we should refer to the load function with ``'s. Try to see Greg 
Mann's proposal on the dev list :)



src/tests/oversubscription_tests.cpp (lines 199 - 201)


We need to do a scan for argument wrapping. Should be 4 space indent.



src/tests/oversubscription_tests.cpp (line 1157)


Instead of refering to BE, we standardized on 'recovable' I think. @Vinod: 
what do you think?



src/tests/oversubscription_tests.cpp (line 1170)


Doesn't this need to be 

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Nov. 26, 2015, 12:05 p.m.)


Review request for mesos and Niklas Nielsen.


Changes
---

Fixed Makefile problem.


Repository: mesos


Description
---

Added Load QoS Controller for the simple eviction when system load is above 
configured threshold:
- Made os::loadavg called from internal method. This method is then passed as 
lambda to the Load QoS Controller process.
- Added MockLoadQoSController
- Added tests

Tests still WIP.


Diffs (updated)
-

  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


Thanks,

Bartek Plotka

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40617/
---

(Updated Nov. 25, 2015, 3:45 p.m.)


Review request for mesos and Niklas Nielsen.


Changes
---

Made os::loadavg called from internal method. This method is then passed as 
lambda to process. Added hpp file for LoadQoSController interface - we need to 
test it first as a single unit. Added MockLoadQoSController, and filled first 
test. Still WIP.


Repository: mesos


Description (updated)
---

Added Load QoS Controller for the simple eviction when system load is above 
configured threshold:
- Made os::loadavg called from internal method. This method is then passed as 
lambda to the Load QoS Controller process.
- Added MockLoadQoSController
- Added tests

Tests still WIP.


Diffs (updated)
-

  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


Thanks,

Bartek Plotka