----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16432/#review33029 -----------------------------------------------------------
This has some issues with not properly doing clean up in an asynchronous manner. See Adam's comments. This got be fixed. Also, I see a lot of CHECKs in this code where a return of Error() or Failure() would've been appropriate. Fix those too? src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment62185> Move this up to the initializer list. src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment62186> const? src/slave/containerizer/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment62188> these should be in reverse order. src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment62195> +1 How about moving this file up to "containerizer/cgroups.cpp" so that it can be used by both launcher and isolators? src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment62201> parameters should be aligned. src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment62202> parameters should be properly aligned. src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment62203> alignment. src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment62196> Reverse order. src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment62211> Why is this a public method? src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment62206> I wager this is because there is already a method by the same name. This has been our convention for name clashes. src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment62207> I wager this is because there is already a method by the same name. This has been our convention for name clashes. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62215> I actually wonder if it's cleaner to just have this class maintain a "vector<string> subsystems" instead of just "string subsystem" and inject cpuacct subsystem here. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62216> +1 src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62217> Please include the container id in the Failure message. More importantly, I don't think this should be Failure. See my comment in an earlier review on Launcher. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62218> How about CpushareCgroupInfo* info = new CpushareCgroupInfo(containerId, flags.cgroup_root); src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62224> you can combine these CpushareCgroupInfo* info = CHECK_NOTNULL(infos[containerId]); src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62225> ditto. you can combine these. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62227> +1. kill this line. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62228> combine these two. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62229> ditto. combine these two. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment62230> +1 src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment62231> Why is this public? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62232> ditto. should not be a failure. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62234> MemCgroupInfo* info = new MemCgroupInfo(containerId, flags.cgroups_root); src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62235> combine these two. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62238> combine these two. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62239> +1 src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62240> combine these two. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment62242> s/ERROR/FATAL/ ? - Vinod Kone On Jan. 27, 2014, 3:03 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16432/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2014, 3:03 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 cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 > 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 > >
