mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilbert Song <songzihao1...@gmail.com>
Subject Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem
Date Thu, 15 Jun 2017 11:14:35 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54693/#review177992
-----------------------------------------------------------




include/mesos/mesos.proto
Lines 2244-2246 (patched)
<https://reviews.apache.org/r/54693/#comment251722>

    Could we elaborate more here, since we have two `Total`:
    1. Operation type `Total`.
    2. At the last line of some cgroup file, we have a summerized value `Total`.
    
    Also, seems like we are going to traverse a cgroup hierarchy only once and collect statistics
all together. The statistics will be devided by devices (or it represents a general `Total`
from the last line of a cgroup file). Then, for the `Cgroup::blkio::Statistics`, we have a
structure like:
    1:0 -> all device stats (CFQ, CFQ_recursive, Throttle)
    1:1 -> all device stats (CFQ, CFQ_recursive, Throttle)
    2:0 -> all device stats (CFQ, CFQ_recursive, Throttle)
    ...
    8:2 -> all device stats (CFQ, CFQ_recursive, Throttle)
    Total -> all summerized stats (CFQ, CFQ_recursive, Throttle)
    
    I am fine with it but we need to be clear in comment. People may be confused about it.



include/mesos/mesos.proto
Lines 2260-2275 (patched)
<https://reviews.apache.org/r/54693/#comment251723>

    Will these fields be used in `ContainerStatus`?
    
    We only include `Blkio::ResourceStatistics` in `ResourceStatistics`. And these fields
are inside of `CgroupInfo`, which means that they will only be set in `Blkio::Subsystem::status()`,
right?
    
    Understand that we want to keep all `blkio` related proto grouped at one place, but we
need to comment that explicitly.



include/mesos/mesos.proto
Lines 2287 (patched)
<https://reviews.apache.org/r/54693/#comment251724>

    As mentioned above, let's comment that this `CgruopInfo::ResourceStatistics` will only
be consumed in `ResourceStatistics` protobuf message, which reports container usages.



include/mesos/mesos.proto
Lines 2287-2290 (patched)
<https://reviews.apache.org/r/54693/#comment251725>

    Move above `NetCls`?


- Gilbert Song


On Dec. 21, 2016, 12:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal
Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> 
> Diff: https://reviews.apache.org/r/54693/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>


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