mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.
Date Tue, 18 Jul 2017 14:27:22 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message