You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Christina Delimitrou <cd...@twitter.com> on 2013/08/28 00:46:00 UTC
Review Request 13868: Exposing the per cgroup cpu.stat statistics through
mesos.container
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.hpp bbbbfd3
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing
-------
Thanks,
Christina Delimitrou
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
(Updated Aug. 30, 2013, 12:30 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs (updated)
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing
-------
make check
Thanks,
Christina Delimitrou
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
> On Aug. 30, 2013, 1:11 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolator.cpp, line 805
> > <https://reviews.apache.org/r/13868/diff/2/?file=346509#file346509line805>
> >
> > This time needs to be converted now, right?
> >
> > Nanoseconds(stat.get()["throttled_time"]).secs()
I think this has to stay this way. The cgroup stat name that it has to read is "throttled_time".
- Christina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25779
-----------------------------------------------------------
On Aug. 30, 2013, 12:30 a.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25779
-----------------------------------------------------------
Each "issue" can be marked resolved or dropped, can you update them? (I really hate the word "issue" because it's pretty jarring for people, but the ability to track whether they were fixed or dropped is nice).
Also, can you replace the usage of tabs with spaces in your diff?
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13868/#comment50306>
This time needs to be converted now, right?
Nanoseconds(stat.get()["throttled_time"]).secs()
- Ben Mahler
On Aug. 30, 2013, 12:30 a.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25883
-----------------------------------------------------------
Did make check pass with this? This appears to have broken MonitorTest.WatchUnwatch, however I've fixed the issue upon committing so please mark this change as submitted!
- Ben Mahler
On Aug. 30, 2013, 11:31 p.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 11:31 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
(Updated Aug. 30, 2013, 11:31 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
s/tab/spaces...
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs (updated)
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing
-------
make check
Thanks,
Christina Delimitrou
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25803
-----------------------------------------------------------
Ship it!
Looks great! Just a minor whitespace nit.
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13868/#comment50337>
replace the tab here with spaces :)
- Ben Mahler
On Aug. 30, 2013, 7:04 p.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 7:04 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
(Updated Aug. 30, 2013, 7:04 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Based on Ben's comment, converting "throttled_time" to seconds.
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs (updated)
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing
-------
make check
Thanks,
Christina Delimitrou
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
(Updated Aug. 30, 2013, 2:52 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
Changes
-------
Based on Ben and Ian's comments. Thanks for the quick replies :)
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs (updated)
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing
-------
make check
Thanks,
Christina Delimitrou
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25783
-----------------------------------------------------------
Ship it!
src/slave/monitor.cpp
<https://reviews.apache.org/r/13868/#comment50310>
kill new line
- Vinod Kone
On Aug. 30, 2013, 12:30 a.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
(Updated Aug. 30, 2013, 12:30 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing
-------
make check
Thanks,
Christina Delimitrou
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25771
-----------------------------------------------------------
Great!
include/mesos/mesos.proto
<https://reviews.apache.org/r/13868/#comment50288>
Can you move this right below the other cpu related stats? Just so we can visually group related stats in the protobuf.
Would be nice to have a unit on cpus_throttled_time.
How about either:
optional double cpus_throttled_time_secs;
optional uint64 cpus_throttled_time_ns;
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13868/#comment50290>
Please kill the trailing whitespace here and elsewhere.
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13868/#comment50298>
How about:
// Add the cpu.stat information.
No need to list them since the code lists them as well :)
src/slave/constants.hpp
<https://reviews.apache.org/r/13868/#comment50291>
Why the move?
src/slave/constants.cpp
<https://reviews.apache.org/r/13868/#comment50295>
You can kill this comment.
Can we collect this information once per second with little performance impact?
src/slave/monitor.cpp
<https://reviews.apache.org/r/13868/#comment50297>
s/TODO/ TODO/
- Ben Mahler
On Aug. 29, 2013, 11:21 p.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2013, 11:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.hpp bbbbfd3
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
> On Aug. 29, 2013, 11:50 p.m., Ian Downes wrote:
> > include/mesos/mesos.proto, line 286
> > <https://reviews.apache.org/r/13868/diff/1/?file=345284#file345284line286>
> >
> > why a double? - why not keep fixed point, say in ms rather than original ns.
I kept it in double to be consistent with the user/kernel cpu times above. I've added the specification that it's in secs though.
- Christina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25770
-----------------------------------------------------------
On Aug. 30, 2013, 12:30 a.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2013, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/#review25770
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/13868/#comment50287>
why a double? - why not keep fixed point, say in ms rather than original ns.
src/slave/cgroups_isolator.cpp
<https://reviews.apache.org/r/13868/#comment50285>
clean up extra whitespace here and elsewhere - it's marked in red here on reviewboard
src/slave/constants.hpp
<https://reviews.apache.org/r/13868/#comment50286>
is this move necessary?
- Ian Downes
On Aug. 29, 2013, 11:21 p.m., Christina Delimitrou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13868/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2013, 11:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bbf1d31
> src/slave/cgroups_isolator.cpp 676768e
> src/slave/constants.hpp bbbbfd3
> src/slave/constants.cpp 8c74c00
> src/slave/monitor.cpp 8e1eb35
> src/slave/process_isolator.cpp fa80293
>
> Diff: https://reviews.apache.org/r/13868/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Christina Delimitrou
>
>
Re: Review Request 13868: Exposing the per cgroup cpu.stat statistics
through mesos.container
Posted by Christina Delimitrou <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13868/
-----------------------------------------------------------
(Updated Aug. 29, 2013, 11:21 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Adding fields to record the cpu.stat statistics per cgroup. These include nr_periods, nr_throttled and throttled_time.
Diffs
-----
include/mesos/mesos.proto bbf1d31
src/slave/cgroups_isolator.cpp 676768e
src/slave/constants.hpp bbbbfd3
src/slave/constants.cpp 8c74c00
src/slave/monitor.cpp 8e1eb35
src/slave/process_isolator.cpp fa80293
Diff: https://reviews.apache.org/r/13868/diff/
Testing (updated)
-------
make check
Thanks,
Christina Delimitrou