----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5174/#review8015 -----------------------------------------------------------
This is a great beginning! It probably makes sense to separate out the mount stuff into it's own patch and get that committed first. Then I'll take another pass on the cgroup stuff alone (as you fill in implementations). Keep up the good work! src/Makefile.am <https://reviews.apache.org/r/5174/#comment17383> Please wrap this line like the others. src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17397> We should provide some high-level documentation at least defining the terms that are being used here (e.g., subsystem, hierarchy, task, etc). src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17384> We put opening '{' for classes/structs on newlines. src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17389> Comment is not necessary. ;) But the need for a default constructor is a little disconcerting (more prone for bugs). src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17385> Indentation of 4 here please. Also, use camel case for variable names (but the leading '_' is okay). src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17399> We should stick the mount related stuff into src/linux/mount.hpp and src/linux/mount.cpp. src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17388> These are very useful comments, but please make them complete sentences too (i.e., capitalize first word in sentence and put a period at the end). src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17387> Kill all extra whitespace in this diff please. src/linux/cgroups.hpp <https://reviews.apache.org/r/5174/#comment17400> s/check/Check s/valid/valid. src/linux/cgroups.cpp <https://reviews.apache.org/r/5174/#comment17390> This should be configurable (because it's configurable by default right?). src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/5174/#comment17386> We should see if there is a clean way to have these run on all (a) Linux machines that (b) have cgroups. Not sure if there are runtime hooks for something like this in gtest/gmock. Otherwise, these tests will need to be DISABLED_ so that they don't break builds on OS X (which obviously is not an ideal solution). - Benjamin On 2012-05-21 01:13:01, Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5174/ > ----------------------------------------------------------- > > (Updated 2012-05-21 01:13:01) > > > Review request for mesos and Benjamin Hindman. > > > Summary > ------- > > Add a few basic functions to control linux cgroups directly. > > > Diffs > ----- > > src/Makefile.am 333234d > src/linux/cgroups.hpp PRE-CREATION > src/linux/cgroups.cpp PRE-CREATION > src/tests/cgroups_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/5174/diff > > > Testing > ------- > > Very basic tests. On linux machine: make check > > > Thanks, > > Jie > >
