----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16432/#review32647 -----------------------------------------------------------
Ship it! Again, good stuff :) A few easy style nits/questions. src/slave/container/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment61529> Can these be set in the initializer list? src/slave/container/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment61530> Wrap and indent :) src/slave/container/isolators/cgroups/cgroups.hpp <https://reviews.apache.org/r/16432/#comment61531> This should fit on one line. src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61532> Does it make sense to use path::join? src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61533> Any particular need to do LOG(FATAL)? src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61534> Same here. src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61535> And here. src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61555> Wrap please. src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61551> CHECK's are usually wrapped and indented: CHECK_SOME(exists) << "Failed to determine if '" << info->croup << "' cgroup already exists in the hierarchy at '" << hierarchy << "': " << exists.error(); src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61536> Any particular need for LOG(FATAL)? src/slave/container/isolators/cgroups/cgroups.cpp <https://reviews.apache.org/r/16432/#comment61539> Fix indentation :) src/slave/container/isolators/cgroups/cpushare.hpp <https://reviews.apache.org/r/16432/#comment61528> Wrap and indent. src/slave/container/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61540> Also here, does it make sense to move to constants.hpp? src/slave/container/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61554> Wrap please :) src/slave/container/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61552> Wrap please. src/slave/container/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61553> Wrap please. src/slave/container/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61549> How about wrapping after '=' ? Try<Nothing> write = cgroups::write(hierarchy, info->cgroup, "cpu.shares", stringify(shares)); src/slave/container/isolators/cgroups/cpushare.cpp <https://reviews.apache.org/r/16432/#comment61556> Wrap please. src/slave/container/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/16432/#comment61550> This could fit on a single line. src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61537> How about moving this to constants.hpp? src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61548> How about wrapping after '='? Try<Nothing> write = cgroups::write(hierarchy, info->cgroup, control, stringify(limitInBytes)); src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61542> Does it make sense to move this to os.hpp in stout? src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61557> Wrap please. src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61546> Any good reason for LOG(FATAL)? src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61545> With the usual wrapping, shouldn't this be: LOG(ERROR) << "Failed to numify 'memory.limit_in_bytes': " << bytes.error(); ? src/slave/container/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/16432/#comment61543> Same here: LOG(ERROR) << "Failed to numify 'memory.usage_in_bytes': " << bytes.error(); ? - Niklas Nielsen On Jan. 21, 2014, 11:35 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16432/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2014, 11:35 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/slave/container/isolators/cgroups/cgroups.hpp PRE-CREATION > src/slave/container/isolators/cgroups/cgroups.cpp PRE-CREATION > src/slave/container/isolators/cgroups/cpushare.hpp PRE-CREATION > src/slave/container/isolators/cgroups/cpushare.cpp PRE-CREATION > src/slave/container/isolators/cgroups/mem.hpp PRE-CREATION > src/slave/container/isolators/cgroups/mem.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/16432/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
