----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13903/#review25815 -----------------------------------------------------------
src/linux/cgroups.hpp <https://reviews.apache.org/r/13903/#comment50354> I'm not sure I like the snake cased function names. We rarely do this (if ever) in our code base. I understand you wanted to match the control file names, but why not have simple function names instead? How about limit(), softLimit() and usage()? The 'bytes' in the function name is redundant because we have 'Bytes' as input/return. Thoughts? src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50355> Why not just do Bytes mem = Megabytes((uint64_t) resource.scalar().value()); ? src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50363> I recommend not doing this big change (always setting soft limit) as part of this review. Lets keep this review to use the new memory abstraction. You can split the soft limit change into another review. This way it would be easy to revert if things do not work as expected. src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50358> s/memory.soft_limit_in_bytes/memory soft limit/ src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50359> s/memory.soft_limit_in_bytes/memory soft limit/ src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50360> s/because/is because/. Also this sentence doesn't follow correctly from the previous sentence. How about phrasing it as it was before? src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50361> s/memory.limit_in_bytes/memory limit/ src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50357> s/memory.limit_in_bytes/memory limit/ src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50356> s/memory.usage_in_bytes/memory usage/ src/slave/cgroups_isolator.cpp <https://reviews.apache.org/r/13903/#comment50362> Why no logging on error like you did above? - Vinod Kone On Aug. 31, 2013, 12:29 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13903/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2013, 12:29 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > I've adjusted the CgroupsIsolator code to use these new functions. > > Note that this always sets the soft_limit_in_bytes. This should have been > done in the first place, so let me know if you see a reason to conditionally > set the soft limit as before. > > > Diffs > ----- > > src/linux/cgroups.hpp 3989712b5b76bc960f03fdf020a4e5b735ba5d80 > src/linux/cgroups.cpp b97a89cafab12aa642645508ad465f714ee08f1b > src/slave/cgroups_isolator.cpp 676768e6b8bd13820467309814845257a9c47e02 > > Diff: https://reviews.apache.org/r/13903/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
