----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90581 -----------------------------------------------------------
Hey Jojy, great work so far! Thanks for writing up the JIRA first ;-) Just doing a first review as I familiarize myself with the code. I think the pattern and style feedback is valuable regardless! :-D Regarding the debate around `Duration`: regardless of whether `Duration` is 'overkill', I think there is value in consistency for the sake of readability. Many of our utility functions use / return `Duration`s and so a user of this code will very likely be familiar with the types of things they can do with a `Duration`. src/linux/cgroups.cpp (lines 2006 - 2010) <https://reviews.apache.org/r/36106/#comment143679> Even though these thoughts are related, if the assignment statement doesn't fit on a single line, we tend to leave a blank line between the assignment and the if block. ``` Try<hashmap<string, uint64_t> > stats = cgroups::stat(hierarchy, cgroup, "cpuacct.stat"); if (!stats.isSome()) { return Error(stats.error()); } ``` Same elsewhere in this patch. Not yours: We can also kill the trailing space for the template specialization :-) src/linux/cgroups.cpp (line 2008) <https://reviews.apache.org/r/36106/#comment143686> `stats.isError()` instead of requiring the negation. Here and elsewhere. src/linux/cgroups.cpp (lines 2016 - 2019) <https://reviews.apache.org/r/36106/#comment143683> Can you add a comment here as to why it's ok to cache this value? (As in this value is not dynamic) What does it mean for this value to be `0`? Should we be aborting here? If we're cache the value, should we only check it once? If so: Maybe it's worth pulling this out into a static function within this module so that we can use it elsewhere. This value seems generally usefull. What do you think? src/linux/cgroups.cpp (line 2029) <https://reviews.apache.org/r/36106/#comment143684> We leave a space after c-style casts `(double) stats` Here and elsehwere. src/tests/cgroups_tests.cpp (line 1192) <https://reviews.apache.org/r/36106/#comment143688> We currently don't allow namespace aliasing. I understand this isn't well documented, sorry about that! src/tests/cgroups_tests.cpp (line 1194) <https://reviews.apache.org/r/36106/#comment143687> Can make this `const`. Side note: This file has a significant amount of `std::string` and `std::vector` and trailing spaces for template specializations. Maybe we can do a clean-up pass after this patch lands :-) - Joris Van Remoortere On July 6, 2015, 6:20 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36106/ > ----------------------------------------------------------- > > (Updated July 6, 2015, 6:20 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and > Timothy Chen. > > > Bugs: MESOS-2961 > https://issues.apache.org/jira/browse/MESOS-2961 > > > Repository: mesos > > > Description > ------- > > cgroups implementation does not have a cpuacct subsystem implementation as of > today. Adding the implementation for stat function. > > Changes: > - added Stats class to encapsulate cpuacct.stat data > - added implementation for cpuacct::stats > - added unit tests > > Jira: MESOS-2961 > > > Diffs > ----- > > src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 > src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 > src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a > > Diff: https://reviews.apache.org/r/36106/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jojy Varghese > >
