You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2012/05/21 03:13:03 UTC
Review Request: Added an interface to control linux cgroups.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5174/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Summary
-------
Add a few basic functions to control linux cgroups directly.
Diffs
-----
src/Makefile.am 333234d
src/linux/cgroups.hpp PRE-CREATION
src/linux/cgroups.cpp PRE-CREATION
src/tests/cgroups_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/5174/diff
Testing
-------
Very basic tests. On linux machine: make check
Thanks,
Jie
Re: Review Request: Added an interface to control linux cgroups.
Posted by Jie Yu <yu...@gmail.com>.
> On 2012-05-21 19:06:11, Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 55
> > <https://reviews.apache.org/r/5174/diff/1/?file=109708#file109708line55>
> >
> > This should be configurable (because it's configurable by default right?).
This is like /proc/stat which I don't think it's configurable. Are you suggesting cgroups hierarchy root (which is configurable)?
> On 2012-05-21 19:06:11, Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 43
> > <https://reviews.apache.org/r/5174/diff/1/?file=109707#file109707line43>
> >
> > Comment is not necessary. ;) But the need for a default constructor is a little disconcerting (more prone for bugs).
Since I will put this structure in an std::map, what should I do to avoid a default constructor here? Or you don't allow structure to be put directly into an stl container (use a pointer instead)?
> On 2012-05-21 19:06:11, Benjamin Hindman wrote:
> > src/Makefile.am, line 195
> > <https://reviews.apache.org/r/5174/diff/1/?file=109706#file109706line195>
> >
> > Please wrap this line like the others.
Do you know any command to do this automatically in vi?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5174/#review8015
-----------------------------------------------------------
On 2012-05-21 01:13:01, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5174/
> -----------------------------------------------------------
>
> (Updated 2012-05-21 01:13:01)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> Add a few basic functions to control linux cgroups directly.
>
>
> Diffs
> -----
>
> src/Makefile.am 333234d
> src/linux/cgroups.hpp PRE-CREATION
> src/linux/cgroups.cpp PRE-CREATION
> src/tests/cgroups_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5174/diff
>
>
> Testing
> -------
>
> Very basic tests. On linux machine: make check
>
>
> Thanks,
>
> Jie
>
>
Re: Review Request: Added an interface to control linux cgroups.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5174/#review8015
-----------------------------------------------------------
This is a great beginning! It probably makes sense to separate out the mount stuff into it's own patch and get that committed first. Then I'll take another pass on the cgroup stuff alone (as you fill in implementations). Keep up the good work!
src/Makefile.am
<https://reviews.apache.org/r/5174/#comment17383>
Please wrap this line like the others.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17397>
We should provide some high-level documentation at least defining the terms that are being used here (e.g., subsystem, hierarchy, task, etc).
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17384>
We put opening '{' for classes/structs on newlines.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17389>
Comment is not necessary. ;) But the need for a default constructor is a little disconcerting (more prone for bugs).
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17385>
Indentation of 4 here please. Also, use camel case for variable names (but the leading '_' is okay).
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17399>
We should stick the mount related stuff into src/linux/mount.hpp and src/linux/mount.cpp.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17388>
These are very useful comments, but please make them complete sentences too (i.e., capitalize first word in sentence and put a period at the end).
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17387>
Kill all extra whitespace in this diff please.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5174/#comment17400>
s/check/Check
s/valid/valid.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5174/#comment17390>
This should be configurable (because it's configurable by default right?).
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5174/#comment17386>
We should see if there is a clean way to have these run on all (a) Linux machines that (b) have cgroups. Not sure if there are runtime hooks for something like this in gtest/gmock. Otherwise, these tests will need to be DISABLED_ so that they don't break builds on OS X (which obviously is not an ideal solution).
- Benjamin
On 2012-05-21 01:13:01, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5174/
> -----------------------------------------------------------
>
> (Updated 2012-05-21 01:13:01)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> Add a few basic functions to control linux cgroups directly.
>
>
> Diffs
> -----
>
> src/Makefile.am 333234d
> src/linux/cgroups.hpp PRE-CREATION
> src/linux/cgroups.cpp PRE-CREATION
> src/tests/cgroups_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5174/diff
>
>
> Testing
> -------
>
> Very basic tests. On linux machine: make check
>
>
> Thanks,
>
> Jie
>
>