----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60933/#review180796 -----------------------------------------------------------
src/linux/cgroups.hpp Lines 422 (patched) <https://reviews.apache.org/r/60933/#comment256047> Suggest to separte this to 3 lines: ``` { return value == that.value; } ``` src/linux/cgroups.hpp Lines 425 (patched) <https://reviews.apache.org/r/60933/#comment256048> Ditto. src/linux/cgroups.hpp Lines 427 (patched) <https://reviews.apache.org/r/60933/#comment256049> Why do we need this? src/linux/cgroups.hpp Lines 429 (patched) <https://reviews.apache.org/r/60933/#comment256050> I think there is no need to have a second `public:` in this class. src/linux/cgroups.hpp Lines 446-462 (patched) <https://reviews.apache.org/r/60933/#comment256071> Can we merge these 2 structures into one by making the field `Operation op` optional? And we could also merge `read_entries()` and `read_stat_entries()` into one since they are pretty similar. And merge `Entry::parse()` and `StatEntry::parse()` into one by adding another parameter to indict whether parsing operation or not. src/linux/cgroups.hpp Lines 457 (patched) <https://reviews.apache.org/r/60933/#comment256057> Why is this an optional field? Any chance there is no device in an entry? src/linux/cgroups.hpp Lines 488 (patched) <https://reviews.apache.org/r/60933/#comment256058> s/blkio requests/BIOS requests/ src/linux/cgroups.hpp Lines 495 (patched) <https://reviews.apache.org/r/60933/#comment256059> Ditto. src/linux/cgroups.hpp Lines 545-546 (patched) <https://reviews.apache.org/r/60933/#comment256061> I see you mentioned `along with devices and types of operations` only for this method but not for others, maybe we should mention it for all methods which returns `Try<std::vector<StatEntry>>`? src/linux/cgroups.hpp Lines 575 (patched) <https://reviews.apache.org/r/60933/#comment256062> Do you want to put all the methods above into a namespace (e.g., `namespace CFQ`)? That seems more aligned with the protobuf messages that you introduced in the previous patch. Or you could delete `message CFQ {` from the previous patch, that seems more consistent with the structure of blkio cgroup (blkio.xxx and blkio.throttle.xxxx) src/linux/cgroups.hpp Lines 575-576 (patched) <https://reviews.apache.org/r/60933/#comment256064> Merge into a single line. src/linux/cgroups.cpp Lines 1939 (patched) <https://reviews.apache.org/r/60933/#comment256065> Kill this empty line. src/linux/cgroups.cpp Lines 1944 (patched) <https://reviews.apache.org/r/60933/#comment256068> Should we replace `unsigned int` with `dev_t` like the code below? https://github.com/apache/mesos/blob/1.3.0/src/linux/cgroups.cpp#L2572 Or actually I think the above code seems wrong, we should change it from `dev_t` to `unsigned int`. src/linux/cgroups.cpp Lines 1946 (patched) <https://reviews.apache.org/r/60933/#comment256069> Suggest to change this message to: ``` "Invalid device major number: '" + device[0] + "'" ``` src/linux/cgroups.cpp Lines 1951 (patched) <https://reviews.apache.org/r/60933/#comment256070> Ditto. src/linux/cgroups.cpp Lines 2035 (patched) <https://reviews.apache.org/r/60933/#comment256072> Kill this empty line. src/linux/cgroups.cpp Lines 2044 (patched) <https://reviews.apache.org/r/60933/#comment256074> Ditto. src/linux/cgroups.cpp Lines 2063 (patched) <https://reviews.apache.org/r/60933/#comment256073> Ditto. src/linux/cgroups.cpp Lines 2072 (patched) <https://reviews.apache.org/r/60933/#comment256075> Ditto. src/linux/cgroups.cpp Lines 2261-2262 (patched) <https://reviews.apache.org/r/60933/#comment256080> Merge into a single line. - Qian Zhang On July 18, 2017, 8:44 a.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60933/ > ----------------------------------------------------------- > > (Updated July 18, 2017, 8:44 a.m.) > > > Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, > Vinod Kone, and Zhitao Li. > > > Bugs: MESOS-6162 > https://issues.apache.org/jira/browse/MESOS-6162 > > > Repository: mesos > > > Description > ------- > > - Data structure for Blkio entities > - Stats helpers for blkio.throttle.io* (generic blkio stats) > - Stats helpers for blkio.io* (CFQ related stats) > - Comments from the kernel blkio doc for helper functions > > > Diffs > ----- > > src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 > src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 > > > Diff: https://reviews.apache.org/r/60933/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Gilbert Song > >
