----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16432/#review33920 -----------------------------------------------------------
Ship it! src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment63705> What's with snake case? src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment63738> It looks like 'setup' is basically "(1) make sure cgroups are supported then (2) make sure hierarchy is mounted with subsystem and (3) make sure there is a root cgroup". I imagine that could just be moved into our cgroups library and then it can be used by the launcher too. src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment63737> It seems like these two can and should just be functions on CgroupInfo. src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment63706> Spelling (and snake case). src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment63707> Kill newline (and looking back I realized you've done this in a fair number of files, please kill trailing newlines elsewhere too). src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment63708> virtual src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment63709> virtual src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment63710> virtual src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment63711> const? src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63712> Newline between these two please. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63713> s/flags_/_flags/ src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63714> You shouldn't need to wrap this with 'string'. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63716> s/exiting/exited/ src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63717> Wrap at 80 columns please. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63727> Anymore cleanup need to be done in this case? src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63728> See comment above. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63718> You can do the CHECK_NOTNULL from above right here. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment63719> You could fire these off in parallel and do a collect too. src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment63720> virtual src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment63721> virtual src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment63722> virtual src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment63723> Add a newline above please. src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment63724> const? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63725> Newline between these please. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63726> Don't think 'string' is needed here. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63729> Anymore cleanup need to be done here? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63730> s/ */* / src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63731> Move CHECK_NOTNULL down here from above. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63732> s/ */* / src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63733> Shouldn't need 'string' here. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63734> Please put the newline back above this comment. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63735> Please wrap at 80 columns. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment63736> Please wrap at 80 columns. - Benjamin Hindman On Feb. 7, 2014, 3:38 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16432/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 3:38 a.m.) > > > Review request for mesos, Benjamin Hindman, Chi Zhang, samya, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Forth part of isolation refactor - cgroup isolators > > CPU and Memory isolators using Linux cgroups. > > This code was written by Chi Zhang <[email protected]> and based on the > original cgroups_isolator. > > > Diffs > ----- > > src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 > src/slave/containerizer/isolators/cgroups/cgroups.hpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/cgroups.cpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/cpushare.hpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/cpushare.cpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/mem.hpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/mem.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/16432/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
