> 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? > > > > Benjamin Hindman wrote: > 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.
I've dropped this one in favor of keeping the methods consistent with the control filenames. Adding on to benh's comment, I think this also makes it very clear as to what controls are being manipulated, which in turn forces the user to read the cgroups documentation to understand how to use the control. We could hide this as you suggested but I prefer to mirror the controls for now in favor of being as explicit as possible (cgroups are complicated so I'm hesitant to hide too many of the details through our API). Let me know what you think! > On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote: > > src/slave/cgroups_isolator.cpp, line 1044 > > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1044> > > > > Why not just do > > > > Bytes mem = Megabytes((uint64_t) resource.scalar().value()); ? This would lose accuracy! Take the example of mem == 0.5: Megabytes((uint64_t) 0.5) == Megabytes(0) > On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote: > > src/slave/cgroups_isolator.cpp, line 1066 > > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1066> > > > > s/memory.soft_limit_in_bytes/memory soft limit/ Dropped these given the function names mirror the controls. > On Aug. 31, 2013, 5:55 a.m., Vinod Kone wrote: > > src/slave/cgroups_isolator.cpp, line 1227 > > <https://reviews.apache.org/r/13903/diff/2/?file=347000#file347000line1227> > > > > Why no logging on error like you did above? > 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. > > Benjamin Hindman wrote: > +1 to delaying this to a later review. This is a bug! But.. it's pretty harmless and I understand your concerns to minimize change so I will revert my fix. - Ben ----------------------------------------------------------- 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 > >
