> On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, line 189 > > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line189> > > > > To be consistent, either making this function static, or remove static > > modifiers from all internal functions.
Done. > On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, line 255 > > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line255> > > > > Should we add a parameter "bool cloneCpuset = true" to this function? Let's only add that in the future if we think we actually have a use case. For now, anytime we create a cgroup in a hierarchy mounted with cpuset someone will have to clone the values, so we might as well perform this functionality. > On Oct. 16, 2012, 8:23 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, line 769 > > <https://reviews.apache.org/r/7620/diff/1/?file=177249#file177249line769> > > > > I am wondering why using "trim" here? That will break one unit test I > > think. This was done in cgroups_isolation_module.cpp, and like the cloneCpusetCpusMem, I realized that everyone would want/have to do this. In general there were a few places where a leading '/' was expected (like in this call to getCgroups) and other places where it was unnecessary. But at the end of the day I didn't like returning cgroup names with the '/' prefix because it could be mistaken for a directory starting at '/' (root). When the path is not prefixed with '/' it makes it more clear that this is a relative "path". Also, I fixed the unit tests in https://reviews.apache.org/r/7622. The one downside to this was that it doesn't return the "root" cgroup (i.e., '/'), but since we weren't returning this anyway I decided I decided it wasn't a big deal. This might need to change in the future though. Our documentation for most (all?) of the public functions expects "relative" paths to the cgroups (from the hierarchy). If we were going to be strict about that then I suppose we would be expecting './' when referring to the "root" cgroup and we should thus always be returning './' for that cgroup. Thoughts? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7620/#review12484 ----------------------------------------------------------- On Oct. 24, 2012, 4:40 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7620/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2012, 4:40 a.m.) > > > Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/linux/cgroups.hpp f0e343d9348075484fa5a4be739a5cef8b9dea21 > src/linux/cgroups.cpp 58e9fec193cf0da39fdbee3ee1891fd716a63bbb > third_party/libprocess/include/stout/path.hpp > e64f8a01d004c5e6513880ed2a1a85d4ec4f9e8b > > Diff: https://reviews.apache.org/r/7620/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
