----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16432/#review32889 -----------------------------------------------------------
Ship it! Looks good, just some minor nits. src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61881> Why not assign "path::join(root, "test")" to a variable, if it's going to be used 3 times in this function? Isn't this test already performed in the cgroups_launcher? Do we really need it in both? (I guess that's what your TODO in the launcher is about.) src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61883> Do you also want to log the info->cgroup string as above? src/slave/containerizer/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61884> s/cgorups/cgroups/ src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment61886> Shouldn't need both <mesos/resources.hpp> and "mesos/resources.hpp" src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment61930> Please use 'subsystem' instead of 'subsystem_' for the member variable. src/slave/containerizer/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment61887> Just because cgroups uses "cpuacct", does that mean we need to keep that archaic name? I think cpuAccountHierarchy would be easier to read. Or cpuShareHierarchy if you'd rather. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61890> Go ahead and use local variable foo_ and foo = foo_.get() for cpuacctHierarchy here like you did above for hierarchy_. We prefer not to use this-> to workaround shadowed variables. src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61894> Please comment somewhere that CFS stands for "Completely Fair Scheduler". My first google search for "cfs cgroups" suggested "Chronic Fatigue Syndrome" support groups. :P src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61901> isNone() instead of !isSome()? src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61913> No need for all this Option<>:: nonsense, you can just use "= Some(pid);", or even just "= pid;" src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61917> s/hierarchy/cpuacctHierarchy/ src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61919> First "CHECK_NOTNULL(infos[containerId]);" ? src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61918> isNone() instead of !isSome() src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61922> Shadows the identical "double cpus = resources.cpus().get();" on line 249; you can probably remove this line and use the preexisting 'cpus' variable src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61923> Can you chain these futures together with .then() to ensure that we destroy both of these hierarchies and delete the containerId/info from infos, even if the futures aren't immediately ready? src/slave/containerizer/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61926> Newline at end of file src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment61925> Apache License statement? src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment61928> alpha-order src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment61931> s/subsystem_/subsystem/ src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment61932> If you want Doxygen to pick this up, you should either use triple-slash "/// @param foo bar" or double-star: /** * Description. * @param foo bar */ src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61933> alpha-order src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61935> isNone src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61941> isNone src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61942> = pid; src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61944> CHECK_NOTNULL(infos[containerId]); first? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61945> isNone() src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61946> Please put the TODO on a new line src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61947> Would it make sense to change the '<=' to '<'? I'd think you could still use limit_in_bytes if the new limit == current. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61948> Use .then() to ensure the proper cleanup occurs, even if destroy isn't immediately ready. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61968> What if read.isNone()? If you assign this to 'usage' above the read.isSome() check, you could use it in the numify call instead of calling trim(read.get()) again. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61970> What if 'usage' wasn't able to be read above (isNone)? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61971> Technically, it's ending namespace slave, then internal, mesos. - Adam B On Jan. 26, 2014, 7:03 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16432/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2014, 7:03 p.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 > >
