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 60933: Added stats/control helpers for the Blkio cgroup subsystem.
Date Thu, 20 Jul 2017 00:25:55 GMT


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 427 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778067#file1778067line427>
> >
> >     Why do we need this?

Yes, so that we can use an object as a `dev_t` key in hashmap.


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 429 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778067#file1778067line429>
> >
> >     I think there is no need to have a second `public:` in this class.

A qualifier to distinguish `inline methods` to `static Try<Device>`. A style choice,
seems more clear to me in this way.


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 446-462 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778067#file1778067line446>
> >
> >     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.

Did thought about it and was regarding that as complex in parse() logic, but merging them
seems more clear.


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 457 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778067#file1778067line457>
> >
> >     Why is this an optional field? Any chance there is no device in an entry?

Two semantics:
```
8:0 Read 27660288
Total 1000
```


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 545-546 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778067#file1778067line545>
> >
> >     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>>`?

I removed `along with devices and types of operations`.


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1944 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778068#file1778068line1944>
> >
> >     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`.
> 
> Gilbert Song wrote:
>     yea, should be `unsigned int` here for major or minor.

https://github.com/apache/mesos/blob/1.3.0/src/linux/cgroups.cpp#L2572 can be fixed later.


- Gilbert


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


On July 19, 2017, 5:18 p.m., Gilbert Song wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> -----------------------------------------------------------
> 
> (Updated July 19, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, 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/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>


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