You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/10/16 21:21:11 UTC
Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/
-----------------------------------------------------------
Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
Description
-------
See summary.
Diffs
-----
src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
Diff: https://reviews.apache.org/r/7620/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12842
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Oct. 26, 2012, 6:10 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2012, 6:10 p.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/
-----------------------------------------------------------
(Updated Oct. 26, 2012, 6:10 p.m.)
Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
Changes
-------
Updates from review comments.
Description
-------
See summary.
Diffs (updated)
-----
src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
Diff: https://reviews.apache.org/r/7620/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 24, 2012, 10:11 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 204
> > <https://reviews.apache.org/r/7620/diff/2/?file=179327#file179327line204>
> >
> > I'd prefer this visually if you killed the newlines between the
> >
> > Try cpu = ...;
> >
> > if (cpu.isError()) {
> > ...
> > }
> >
> > to become:
> >
> > Try cpu = ...;
> > if (cpu.isError()) {
> > ...
> > }
> >
> > In all the error checks in this function, it makes each logical chunk more obvious IMO.
Okay, I changed this one, but a little reluctantly. In most circumstances I completely agree, but there is a lot of text here and by merging these together makes the important calls no longer "pop" (e.g., readControl, readControl, writeControl, writeControl). In general I'd prefer we use the more compact style unless it makes finding the "meat" of the function more difficult.
> On Oct. 24, 2012, 10:11 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 330
> > <https://reviews.apache.org/r/7620/diff/2/?file=179327#file179327line330>
> >
> > Is jie's TODO here still applicable?
> >
> >
I think he's referring to getting errno out of std::ostringstream. I'm going to leave that for now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12726
-----------------------------------------------------------
On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:40 a.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12726
-----------------------------------------------------------
Ship it!
Cosmetics, so pending my comments, looks good.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27125>
1. Maybe let's verify that it's a relative path?
Oh I see, so passing in "foo" or "/foo" will consider the parent to be "/", or, the "root" of the heirarchy:
/heirarchy//
third_party/libprocess/include/stout/path.hpp
<https://reviews.apache.org/r/7620/#comment27126>
How about:
join(path1, join(path2, path3, path4));
Either in this or the subsequent path::join review you sent out.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27127>
Restore the comment alignment.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27128>
I'd prefer this visually if you killed the newlines between the
Try cpu = ...;
if (cpu.isError()) {
...
}
to become:
Try cpu = ...;
if (cpu.isError()) {
...
}
In all the error checks in this function, it makes each logical chunk more obvious IMO.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27129>
Kill newline?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27131>
Is jie's TODO here still applicable?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27132>
ditto.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment27134>
Not yours but:
s/walk/walk: /
Also looks like this fits on one line.
- Ben Mahler
On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:40 a.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/
-----------------------------------------------------------
(Updated Oct. 24, 2012, 4:40 a.m.)
Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
Changes
-------
Updates based on review comments.
Description
-------
See summary.
Diffs (updated)
-----
src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
Diff: https://reviews.apache.org/r/7620/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 18, 2012, 5:09 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 194
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line194>
> >
> > Any reason for the 'cgroups::' prefix on the read/writeControl calls here?
Bad copy-pasta, thanks.
> On Oct. 18, 2012, 5:09 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 256
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line256>
> >
> > 1. The call to path::join("/", cgroup) is quite strange, it will essentially just prepend a "/" to the cgroup relative path, which seems wrong to me.
> >
> > 2. Is it expected that there's always a parent in this call? Otherwise seems
First, every hierarchy has a "root" cgroup at 'hierarchy' (i.e., 'path::join(hierarchy, "/")'). So by definition, yes, every cgroup has a parent.
That being said, someone could be creating a cgroup called "foo" or "/foo" (since we allow both right now, even though we specify that we want a relative path). In the former case (the "good" relative path), I needed to be able to determine the parent cgroup so I know what cgroup to clone from. Simply calling dirname on this should return '.', which I could make work, but that would require updating a lot of code which doesn't currently call os::realpath to do so. I decided to save that for another day.
> On Oct. 18, 2012, 5:09 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/path.hpp, line 19
> > <https://reviews.apache.org/r/7620/diff/1/?file=177250#file177250line19>
> >
> > Thanks for this, I seem to recall making some clunky join calls that I should fix.
Yup.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12573
-----------------------------------------------------------
On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:40 a.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12573
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26843>
Any reason for the 'cgroups::' prefix on the read/writeControl calls here?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26844>
1. The call to path::join("/", cgroup) is quite strange, it will essentially just prepend a "/" to the cgroup relative path, which seems wrong to me.
2. Is it expected that there's always a parent in this call? Otherwise seems
third_party/libprocess/include/stout/path.hpp
<https://reviews.apache.org/r/7620/#comment26845>
Thanks for this, I seem to recall making some clunky join calls that I should fix.
- Ben Mahler
On Oct. 16, 2012, 7:21 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2012, 7:21 p.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 18, 2012, 1:25 a.m., Vinod Kone wrote:
> > src/linux/cgroups.cpp, line 723
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line723>
> >
> > this is a cpp, so why not just do 'using std::string' at the top.
I agree, cleaned this up, thanks for the nudge!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12553
-----------------------------------------------------------
On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:40 a.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12553
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26744>
'if necessary' sounds ambiguous. you probably meant, if present in the parent hierarchy?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26745>
this is a cpp, so why not just do 'using std::string' at the top.
- Vinod Kone
On Oct. 16, 2012, 7:21 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2012, 7:21 p.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 769
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line769>
> >
> > I am wondering why using "trim" here? That will break one unit test I think.
>
> Benjamin Hindman wrote:
> This was done in cgroups_isolation_module.cpp, and like the cloneCpusetCpusMem, I realized that everyone would want/have to do this. In general there were a few places where a leading '/' was expected (like in this call to getCgroups) and other places where it was unnecessary. But at the end of the day I didn't like returning cgroup names with the '/' prefix because it could be mistaken for a directory starting at '/' (root). When the path is not prefixed with '/' it makes it more clear that this is a relative "path". Also, I fixed the unit tests in https://reviews.apache.org/r/7622.
>
> The one downside to this was that it doesn't return the "root" cgroup (i.e., '/'), but since we weren't returning this anyway I decided I decided it wasn't a big deal. This might need to change in the future though.
>
> Our documentation for most (all?) of the public functions expects "relative" paths to the cgroups (from the hierarchy). If we were going to be strict about that then I suppose we would be expecting './' when referring to the "root" cgroup and we should thus always be returning './' for that cgroup. Thoughts?
Sounds good to me. Thanks for clarifying it.
> it doesn't return the "root" cgroup (i.e., '/')
I agree with you. I don't think this is a problem because we can always find the root cgroup if we want.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12484
-----------------------------------------------------------
On Oct. 26, 2012, 6:10 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2012, 6:10 p.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 189
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line189>
> >
> > To be consistent, either making this function static, or remove static modifiers from all internal functions.
Done.
> On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 255
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line255>
> >
> > Should we add a parameter "bool cloneCpuset = true" to this function?
Let's only add that in the future if we think we actually have a use case. For now, anytime we create a cgroup in a hierarchy mounted with cpuset someone will have to clone the values, so we might as well perform this functionality.
> On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 769
> > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line769>
> >
> > I am wondering why using "trim" here? That will break one unit test I think.
This was done in cgroups_isolation_module.cpp, and like the cloneCpusetCpusMem, I realized that everyone would want/have to do this. In general there were a few places where a leading '/' was expected (like in this call to getCgroups) and other places where it was unnecessary. But at the end of the day I didn't like returning cgroup names with the '/' prefix because it could be mistaken for a directory starting at '/' (root). When the path is not prefixed with '/' it makes it more clear that this is a relative "path". Also, I fixed the unit tests in https://reviews.apache.org/r/7622.
The one downside to this was that it doesn't return the "root" cgroup (i.e., '/'), but since we weren't returning this anyway I decided I decided it wasn't a big deal. This might need to change in the future though.
Our documentation for most (all?) of the public functions expects "relative" paths to the cgroups (from the hierarchy). If we were going to be strict about that then I suppose we would be expecting './' when referring to the "root" cgroup and we should thus always be returning './' for that cgroup. Thoughts?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12484
-----------------------------------------------------------
On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:40 a.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Added cgroups::hierarchies to list the currently active
hierarchies and cgroups::internal::cloneCpusetValues in order to copy
cpuset.cpus and cpuset.mems from a parent to a child cgroup. Also updated
code to use path::join as appropriate.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7620/#review12484
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26578>
To be consistent, either making this function static, or remove static modifiers from all internal functions.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26577>
Should we add a parameter "bool cloneCpuset = true" to this function?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7620/#comment26580>
I am wondering why using "trim" here? That will break one unit test I think.
- Jie Yu
On Oct. 16, 2012, 7:21 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7620/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2012, 7:21 p.m.)
>
>
> Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21
> src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb
> third_party/libprocess/include/stout/path.hpp e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b
>
> Diff: https://reviews.apache.org/r/7620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>