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/07/03 18:03:17 UTC
Re: Review Request: Add APIs for the cgroups freezer subsystem.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5401/#review8835
-----------------------------------------------------------
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5401/#comment18751>
Let's just return a future.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/5401/#comment18752>
Ditto.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18763>
A previous review added the process namespace, so you can clean this code up (and all other related reviews) and drop 'process::' everywhere (and s/std::string/string/ too iirc).
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18753>
virtual
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18754>
Also virtual.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18755>
As in the other reviews, if you can get away with doing terminate where appropriate, and using initialize and finalize than you lower the complexity of the code.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18762>
const &
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18764>
Does this need to be a dispatch?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18767>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18765>
Does this need to be a dispatch?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18761>
const &
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18766>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18768>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18760>
Do you need to dispatch?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18759>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18758>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18757>
terminate
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18756>
Kill this.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18769>
Again, if this stuff was passed into the constructor you can eliminate the extra dispatch.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18771>
Move '*' next to type.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/5401/#comment18770>
Ditto from 'freezeCgroup'.
src/tests/cgroups_tests.cpp
<https://reviews.apache.org/r/5401/#comment18772>
Put expected value first (here and everywhere else please).
- Benjamin Hindman
On June 21, 2012, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5401/
> -----------------------------------------------------------
>
> (Updated June 21, 2012, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This patch provides APIs for controlling the cgroups freezer subsystem.
>
> It is very useful when a slave wants to kill/pause all the processes in (or forked by) an executor.
>
> For example, when an out-of-memory event happens, the slave could choose 1) kill the executor, 2) pause the executor and notify the user, and so on.
>
>
> Diffs
> -----
>
> 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/5401/diff/
>
>
> Testing
> -------
>
> On Linux machine, make check.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add APIs for the cgroups freezer subsystem.
Posted by Jie Yu <yu...@gmail.com>.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 253
> > <https://reviews.apache.org/r/5401/diff/4/?file=115068#file115068line253>
> >
> > Let's just return a future.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.hpp, line 267
> > <https://reviews.apache.org/r/5401/diff/4/?file=115068#file115068line267>
> >
> > Ditto.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 761
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line761>
> >
> > A previous review added the process namespace, so you can clean this code up (and all other related reviews) and drop 'process::' everywhere (and s/std::string/string/ too iirc).
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 769
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line769>
> >
> > virtual
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 771
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line771>
> >
> > Also virtual.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 774
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line774>
> >
> > As in the other reviews, if you can get away with doing terminate where appropriate, and using initialize and finalize than you lower the complexity of the code.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 785
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line785>
> >
> > const &
Done
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 795
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line795>
> >
> > Does this need to be a dispatch?
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 808
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line808>
> >
> > terminate
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 810
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line810>
> >
> > Does this need to be a dispatch?
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 815
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line815>
> >
> > const &
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 822
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line822>
> >
> > terminate
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 826
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line826>
> >
> > terminate
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 843
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line843>
> >
> > Do you need to dispatch?
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 849
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line849>
> >
> > terminate
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 861
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line861>
> >
> > terminate
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 865
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line865>
> >
> > terminate
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 872
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line872>
> >
> > Kill this.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 911
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line911>
> >
> > Again, if this stuff was passed into the constructor you can eliminate the extra dispatch.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 935
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line935>
> >
> > Move '*' next to type.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/linux/cgroups.cpp, line 938
> > <https://reviews.apache.org/r/5401/diff/4/?file=115069#file115069line938>
> >
> > Ditto from 'freezeCgroup'.
Done.
> On July 3, 2012, 4:03 p.m., Benjamin Hindman wrote:
> > src/tests/cgroups_tests.cpp, line 398
> > <https://reviews.apache.org/r/5401/diff/4/?file=115070#file115070line398>
> >
> > Put expected value first (here and everywhere else please).
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5401/#review8835
-----------------------------------------------------------
On June 21, 2012, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5401/
> -----------------------------------------------------------
>
> (Updated June 21, 2012, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This patch provides APIs for controlling the cgroups freezer subsystem.
>
> It is very useful when a slave wants to kill/pause all the processes in (or forked by) an executor.
>
> For example, when an out-of-memory event happens, the slave could choose 1) kill the executor, 2) pause the executor and notify the user, and so on.
>
>
> Diffs
> -----
>
> 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/5401/diff/
>
>
> Testing
> -------
>
> On Linux machine, make check.
>
>
> Thanks,
>
> Jie Yu
>
>