> On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote: > > src/linux/cgroups.hpp, line 386 > > <https://reviews.apache.org/r/13903/diff/2/?file=346998#file346998line386> > > > > 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? > >
Just like with flags, I like the idea of naming the functions to match the underlying file names. I think this will pay off in the long run. > On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote: > > src/slave/cgroups_isolator.cpp, lines 1061-1068 > > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1061> > > > > 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. +1 to delaying this to a later review. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13903/#review25815 ----------------------------------------------------------- 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 > >
