----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5211/#review8069 -----------------------------------------------------------
This is a review for everything but the proc.hpp/cpp stuff (which doesn't look like it's being used by this review anyway, so it might make sense to toss it into a different review). src/linux/cgroups.hpp <https://reviews.apache.org/r/5211/#comment17550> Is there any reason for the hierarchy? For example, are any controls applied in a hierarchical fashion? Any details like this would be great to add for posterity. src/linux/cgroups.hpp <https://reviews.apache.org/r/5211/#comment17551> What about calling it a 'control' instead? Has the cgroups community adopted any terminology themselves? Is it knob? src/linux/cgroups.hpp <https://reviews.apache.org/r/5211/#comment17552> s/WH/Wh src/linux/cgroups.hpp <https://reviews.apache.org/r/5211/#comment17562> This is awesome, but what about two sub-namespaces: 'hierarchies' and 'controls'. E.g., from a clients perspective: cgroups::hierarchies::create(...); cgroups::hierarchies::destroy(...); cgroups::create(hierarchy, cgroup); cgroups::controls::read(hierarchy, cgroup, control); cgroups::controls::write(...); src/linux/cgroups.hpp <https://reviews.apache.org/r/5211/#comment17563> Maybe it makes sense to take a third argument that specifies whether to create the hierarchy if it doesn't exist defaulted to true? src/linux/cgroups.hpp <https://reviews.apache.org/r/5211/#comment17561> s/val/value/ src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17554> s/mount_rc/result src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17555> It would be great to include the mount error too (for easier debugging by the human trying to use this stuff). For example, maybe: return Try<bool>::error("Failed to create hierarchy " + hierarchy + ": " + result.error()); I think the error will include the "Failed to mount" bit, so no need to repeat that here (in fact, in many cases you can get away with something like 'return Try<bool>::error(result.error());'. src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17556> We don't use snake case, except when working with protobuf generated code: s/unmount_rc/result/ is probably better. Also, does it make sense to make the flags passed to unmount default to 0? src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17557> Again, the actual unmount error would probably be helpful here. src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17558> Why use ::mkdir? If you want to make sure it already doesn't exist, you should just do os::exists first. If it's because you want better error information, it probably makes sense to update os::mkdir and have it return a Try. Either way, you can defer this to a later review but add a comment (either here or in utils.hpp next to mkdir and rmdir and any of the other ones that were insufficient for your use case). src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17564> This indentation should be +4 please. src/linux/cgroups.cpp <https://reviews.apache.org/r/5211/#comment17559> s/val/value/ src/linux/mount.hpp <https://reviews.apache.org/r/5211/#comment17553> Any reason not to make this a 'const string&'? src/linux/proc.hpp <https://reviews.apache.org/r/5211/#comment17565> Indent is +4 here please. src/linux/proc.hpp <https://reviews.apache.org/r/5211/#comment17566> No need for the typedef, we prefer explicit types as much as possible. src/linux/proc.hpp <https://reviews.apache.org/r/5211/#comment17567> You should just call this 'entries'. - Benjamin On 2012-05-23 22:30:37, Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5211/ > ----------------------------------------------------------- > > (Updated 2012-05-23 22:30:37) > > > Review request for mesos and Benjamin Hindman. > > > Summary > ------- > > This patch contains the following work: > > 1) Adding interfaces to control linux cgroups directly (see > src/linux/cgroups.h|cpp) > > 2) Adding interfaces to mount/unmount file systems on linux (see > src/linux/mount.h|cpp) > > 3) Adding more interfaces in src/linux/proc to examine cgroups info and mount > table. > > > Diffs > ----- > > src/Makefile.am d6eff8a > src/linux/cgroups.hpp PRE-CREATION > src/linux/cgroups.cpp PRE-CREATION > src/linux/mount.hpp PRE-CREATION > src/linux/mount.cpp PRE-CREATION > src/linux/proc.hpp 2f5c02a > src/linux/proc.cpp ed1beff > src/tests/cgroups_tests.cpp PRE-CREATION > src/tests/mount_tests.cpp PRE-CREATION > src/tests/proc_tests.cpp b9dcb91 > > Diff: https://reviews.apache.org/r/5211/diff > > > Testing > ------- > > Manual tests are done for those operations that need root permission. > > Proc related tests are done in google test framework (ProcTest). > > On linux machines, make check. > > > Thanks, > > Jie > >
