----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5509/#review9717 -----------------------------------------------------------
src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20695> You have an invariant right now that 'resources' will always contain a Value for "cpus". Make that invariant explicit with a CHECK, or at least don't use Value::Scalar() as the "default" and instead call the version of 'get' that returns an Option and don't set the control if the resource doesn't end up existing (e.g., wrap the rest of the function body in a 'if (cpus.isSome())'). If you do the latter, please add a LOG(WARNING) when 'cpus.isNone()'. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20697> Ditto above. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20702> This probably should not be a CHECK (i.e., a processExited could occur before an oomWaited). In addition, it probably makes sense to "tag" each independent framework/executor cgroup so that we can distinguish old events that might fire after we have relaunched a new framework/executor that has the same ID values. I think it makes sense to put this "tag" in the cgroup name as well. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20700> s/destroy/destroyed/ src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/5509/#comment20699> Is there a specific reason why you did executor id first? I'd prefer framework then executor. - Benjamin Hindman On Aug. 1, 2012, 5:33 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5509/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2012, 5:33 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > Add the cgroups isolation module. > > > Diffs > ----- > > src/Makefile.am 10f1101 > src/examples/balloon_executor.cpp PRE-CREATION > src/examples/balloon_framework.cpp PRE-CREATION > src/examples/utils.hpp PRE-CREATION > src/slave/cgroups_isolation_module.hpp PRE-CREATION > src/slave/cgroups_isolation_module.cpp PRE-CREATION > src/slave/isolation_module.cpp 5b7b4a2 > src/tests/cgroups_isolation_tests.cpp PRE-CREATION > src/tests/external/CgroupsIsolation/ROOT_CGROUPS_BalloonFramework.sh > PRE-CREATION > > Diff: https://reviews.apache.org/r/5509/diff/ > > > Testing > ------- > > make check (external test, including OOM tests). > > > Thanks, > > Jie Yu > >
