You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/06/09 20:54:45 UTC
Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
-------
Refactored the ResourceMonitor to get statistics from the Slave.
1) Modified ResourceUsage to include allocation information (see ticket for reaons).
2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
3) Adjusted (and simplified) the MonitorTest accordingly.
Diffs
-----
include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35260/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
> On June 9, 2015, 10:04 p.m., Niklas Nielsen wrote:
> > src/slave/monitor.hpp, line 53
> > <https://reviews.apache.org/r/35260/diff/1/?file=981665#file981665line53>
> >
> > Want to add a deprecation todo?
Chatted with BenM about this. He thinks that we may still want to keep resource monitor in the future if someone wants to keep a timeseries in ResourceMonitor for webui, etc.
> On June 9, 2015, 10:04 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, line 4245
> > <https://reviews.apache.org/r/35260/diff/1/?file=981671#file981671line4245>
> >
> > future->isReady() will always be false here. Is this what you want?
Good catch! Thanks!
> On June 9, 2015, 10:04 p.m., Niklas Nielsen wrote:
> > src/tests/monitor_tests.cpp, lines 204-206
> > <https://reviews.apache.org/r/35260/diff/1/?file=981674#file981674line204>
> >
> > This doesn't test the same thing anymore? Shouldn't this be moved to a slave test instead of getting transformed into another test?
I'll keep this test and add a SlaveTest in a new review.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87300
-----------------------------------------------------------
On June 9, 2015, 6:54 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 6:54 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
> On June 9, 2015, 10:04 p.m., Niklas Nielsen wrote:
> > src/tests/monitor_tests.cpp, lines 204-206
> > <https://reviews.apache.org/r/35260/diff/1/?file=981674#file981674line204>
> >
> > This doesn't test the same thing anymore? Shouldn't this be moved to a slave test instead of getting transformed into another test?
>
> Jie Yu wrote:
> I'll keep this test and add a SlaveTest in a new review.
Added a SlaveTest (https://reviews.apache.org/r/35305/)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87300
-----------------------------------------------------------
On June 10, 2015, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 6:29 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87300
-----------------------------------------------------------
One general comment: Should it be ResourceUsage::Executor or should we be a bit more specific, for example ResourceUsage::ExecutorUsage? You had one name conflict below where you had to call it 'entry'.
I am fine with either.
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139631>
s/allcoated/allocated/
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139632>
s/missing/absent/
s/isolation module/containerizer/
src/slave/monitor.hpp
<https://reviews.apache.org/r/35260/#comment139637>
Want to add a deprecation todo?
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139634>
Can you kill this?
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139633>
Can you kill this too?
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139635>
2 additional space indent as it is a argument wrap
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139636>
Do you know if it makes sense to const lambda::function<>?
src/slave/slave.hpp
<https://reviews.apache.org/r/35260/#comment139638>
Should this also be virtual (taken it is public, which I assume is for testing purposes?)
Maybe not needed yet, so no biggie.
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139640>
Mind adding a comment about the constraint?
I can see you check A == B == C, but was a bit implicit.
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139641>
future->isReady() will always be false here. Is this what you want?
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35260/#comment139646>
This doesn't test the same thing anymore? Shouldn't this be moved to a slave test instead of getting transformed into another test?
- Niklas Nielsen
On June 9, 2015, 11:54 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 11:54 a.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87272
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35259, 35260]
All tests passed.
- Mesos ReviewBot
On June 9, 2015, 6:54 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 6:54 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87440
-----------------------------------------------------------
Ship it!
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139754>
log the error?
- Vinod Kone
On June 10, 2015, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 6:29 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
> On June 10, 2015, 9:52 p.m., Ben Mahler wrote:
> > src/slave/resource_estimators/fixed.cpp, line 60
> > <https://reviews.apache.org/r/35260/diff/2/?file=982137#file982137line60>
> >
> > Whoops! Remove the reference?
This is fixed in https://reviews.apache.org/r/35320/
> On June 10, 2015, 9:52 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 4197
> > <https://reviews.apache.org/r/35260/diff/2/?file=982141#file982141line4197>
> >
> > Seems ok for now, but a bit odd that we're copying it in the capture, since ideally we want to move.
Yeah, I'll add a NOTE here.
> On June 10, 2015, 9:52 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 4213-4215
> > <https://reviews.apache.org/r/35260/diff/2/?file=982141#file982141line4213>
> >
> > Want a CHECK_EQ on the sizes?
Added.
> On June 10, 2015, 9:52 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 4224-4226
> > <https://reviews.apache.org/r/35260/diff/2/?file=982141#file982141line4224>
> >
> > I know we're inconsistent about this, but we should probably single quote the ID here, since it's coming from the framework.
Done.
> On June 10, 2015, 9:52 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 4462
> > <https://reviews.apache.org/r/35260/diff/2/?file=982141#file982141line4462>
> >
> > Trivial, but wrapping after "Fix" looks less jagged?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87459
-----------------------------------------------------------
On June 10, 2015, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 6:29 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87459
-----------------------------------------------------------
Ship it!
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139771>
Feel free to remove this line now.
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139787>
Do you need to copy all the data out of the future? Why not take a const & here or just inline it in the loop:
```
foreach (const ResourceUsage::Executor& executor,
future.get().executors()) {
```
I see that this is 81 characters which is probably why you pulled it out, when we have an -> operator for Future it will fit, oh well. :)
src/slave/resource_estimators/fixed.cpp
<https://reviews.apache.org/r/35260/#comment139780>
Whoops! Remove the reference?
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139776>
Seems ok for now, but a bit odd that we're copying it in the capture, since ideally we want to move.
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139775>
Want a CHECK_EQ on the sizes?
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139778>
I know we're inconsistent about this, but we should probably single quote the ID here, since it's coming from the framework.
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139779>
Trivial, but wrapping after "Fix" looks less jagged?
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35260/#comment139783>
I think we're trying to avoid introducing more of these clauses where we pull in all of 'process', but since we don't have namespace aliases yet, this seems ok.
- Ben Mahler
On June 10, 2015, 6:29 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 6:29 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
> On June 11, 2015, 4:18 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 626
> > <https://reviews.apache.org/r/35260/diff/3/?file=982288#file982288line626>
> >
> > I just thought of one thing - this represents the entire container and not the executor alone. Does it make sense to call it 'Container' instead?
>
> Vinod Kone wrote:
> +1
OK, This is not consistent in the code base. We use "Executor" in master and slave to represent the entire executor (executor itself + its tasks). For example, in master, we have executor->addTask. In slave, we have executor->resources which contains resources not only for itself, but also its tasks.
I prefer the name 'Executor' for consistency (e.g., executorLaunched, executorTerminated, etc.).
@bmahler
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87571
-----------------------------------------------------------
On June 10, 2015, 11:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 11, 2015, 4:18 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 626
> > <https://reviews.apache.org/r/35260/diff/3/?file=982288#file982288line626>
> >
> > I just thought of one thing - this represents the entire container and not the executor alone. Does it make sense to call it 'Container' instead?
+1
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87571
-----------------------------------------------------------
On June 10, 2015, 11:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 11, 2015, 4:18 p.m., Niklas Nielsen wrote:
> > include/mesos/mesos.proto, line 626
> > <https://reviews.apache.org/r/35260/diff/3/?file=982288#file982288line626>
> >
> > I just thought of one thing - this represents the entire container and not the executor alone. Does it make sense to call it 'Container' instead?
>
> Vinod Kone wrote:
> +1
>
> Jie Yu wrote:
> OK, This is not consistent in the code base. We use "Executor" in master and slave to represent the entire executor (executor itself + its tasks). For example, in master, we have executor->addTask. In slave, we have executor->resources which contains resources not only for itself, but also its tasks.
>
> I prefer the name 'Executor' for consistency (e.g., executorLaunched, executorTerminated, etc.).
>
> @bmahler
kk. looks like we call just the executor as ExecutorInfo in our protobufs.
sgtm.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87571
-----------------------------------------------------------
On June 10, 2015, 11:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87571
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139959>
I just thought of one thing - this represents the entire container and not the executor alone. Does it make sense to call it 'Container' instead?
- Niklas Nielsen
On June 10, 2015, 4:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
> On June 11, 2015, 6:12 p.m., Niklas Nielsen wrote:
> > src/slave/monitor.cpp, line 23
> > <https://reviews.apache.org/r/35260/diff/3/?file=982291#file982291line23>
> >
> > C-style headers should be before C++ - take a look at zookeeper.cpp
I think glog/logging.h is a C++ header (google never uses hpp I remember) :)
I'll fix the zookeeper.cpp
> On June 11, 2015, 6:12 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, lines 4218-4219
> > <https://reviews.apache.org/r/35260/diff/3/?file=982296#file982296line4218>
> >
> > I am not sure about lambda wrappings, but shouldn't it as least be 4 indent taken a argument wrapping?
> >
> > Can we prevent that wrapping by wrapping a bit earlier?
Restructured.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87592
-----------------------------------------------------------
On June 10, 2015, 11:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87592
-----------------------------------------------------------
Ship it!
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139991>
Does this represent the total resources allocated for the container (executor + tasks) or only tasks? If the first; then let's update this comment
src/slave/monitor.cpp
<https://reviews.apache.org/r/35260/#comment139993>
C-style headers should be before C++ - take a look at zookeeper.cpp
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139994>
I am not sure about lambda wrappings, but shouldn't it as least be 4 indent taken a argument wrapping?
Can we prevent that wrapping by wrapping a bit earlier?
src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/35260/#comment139995>
What do you need to copy in this lambda scope? I just saw a comment from BenH about preferring being explicit on bringing the environment in.
The style guide suggests that you can do either; I will let that be up to you.
- Niklas Nielsen
On June 10, 2015, 4:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 10, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
> src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/
-----------------------------------------------------------
(Updated June 10, 2015, 11:13 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Changes
-------
Review comments.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
-------
Refactored the ResourceMonitor to get statistics from the Slave.
1) Modified ResourceUsage to include allocation information (see ticket for reaons).
2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
3) Adjusted (and simplified) the MonitorTest accordingly.
Diffs (updated)
-----
include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8
src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35260/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/
-----------------------------------------------------------
(Updated June 10, 2015, 6:29 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed review comments.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
-------
Refactored the ResourceMonitor to get statistics from the Slave.
1) Modified ResourceUsage to include allocation information (see ticket for reaons).
2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
3) Adjusted (and simplified) the MonitorTest accordingly.
Diffs (updated)
-----
include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35260/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Jie Yu <yu...@gmail.com>.
> On June 9, 2015, 11:49 p.m., Ben Mahler wrote:
> > Would be nice to split apart this diff to make it easier to review thoroughly. For example, the following seem to fit into separate patches:
> >
> > (1) Adding Slave::usage for resource estimators.
> > (2) Updating the monitor to use Slave::usage.
> > (3) Moving ResourceMonitorProcess into .cpp file.
> >
> > Also curious if we can use a lambda for the '`Slave::usage`' continuation?
I did want to split the patches for this when I started. I found it quite difficult because we are changing the protobuf `ResourceUsage` which is used by both Slave, ResourceMonitor and ResourceEstimator. That means we cannot do (1) and (2) in separate patches.
For (3), I should have done that in a separate patch. It's a bit hard to do the split right now given the dependencies. Since most of (3) is code movement, could you please do another pass over this patch? Sorry about that, and Thanks for the review.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87270
-----------------------------------------------------------
On June 9, 2015, 6:54 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 6:54 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 35260: Refactored the ResourceMonitor to get
statistics from the Slave.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87270
-----------------------------------------------------------
Would be nice to split apart this diff to make it easier to review thoroughly. For example, the following seem to fit into separate patches:
(1) Adding Slave::usage for resource estimators.
(2) Updating the monitor to use Slave::usage.
(3) Moving ResourceMonitorProcess into .cpp file.
Also curious if we can use a lambda for the '`Slave::usage`' continuation?
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139582>
Remove this comment?
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139583>
This seems pretty close to what the source says ('allocated' Resources can be inferred), how about saying that this includes the resources for its active tasks?
include/mesos/mesos.proto
<https://reviews.apache.org/r/35260/#comment139584>
Which isolation module? :)
I think this comment was copied over from when we had a single isolator (CgroupsIsolator) and no containerizer. How about updating it to say "containerizer" instead?
src/slave/monitor.hpp
<https://reviews.apache.org/r/35260/#comment139585>
Feel free to kill my TODO here and let's update the comment:
```
// Exposes resources usage information via a JSON endpoint.
```
src/slave/slave.cpp
<https://reviews.apache.org/r/35260/#comment139657>
Does this need to be a member function? Looks like you're feeding in everything you need.
Is it possible to use a lambda for this?
```
{
ResourceUsage usage;
list<Future<ResourceStatistics>> futures;
foreachvalue (const Framework* framework, frameworks) {
foreachvalue (const Executor* executor, framework->executors) {
ResourceUsage::Executor* entry = usage.add_executor();
entry->mutable_executor_info()->CopyFrom(executor->info);
entry->mutable_allocated()->CopyFrom(executor->resources);
futures.push_back(containerizer->usage(executor->containerId));
}
}
return await(futures)
.then([&usage, &futures]() {
size_t i = 0;
foreach (const Future<ResourceStatistics> statistics, futures) {
if (future.isReady() {
usage.mutable_executor(i++)->mutable_statistics()->CopyFrom(future.get());
} else {
LOG(WARNING) << ...;
}
}
return usage;
});
```
Might be able to cut out a lot of the boilerplate?
- Ben Mahler
On June 9, 2015, 6:54 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35260/
> -----------------------------------------------------------
>
> (Updated June 9, 2015, 6:54 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2818
> https://issues.apache.org/jira/browse/MESOS-2818
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactored the ResourceMonitor to get statistics from the Slave.
>
> 1) Modified ResourceUsage to include allocation information (see ticket for reaons).
> 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics.
> 3) Adjusted (and simplified) the MonitorTest accordingly.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
> include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd
> src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
> src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
> src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036
> src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb
> src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20
> src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
> src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
> src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
> src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
> src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
>
> Diff: https://reviews.apache.org/r/35260/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>