You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2017/07/18 00:44:33 UTC
Review Request 60933: Added stats/control helpers for the Blkio cgroup
subsystem.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60933/
-----------------------------------------------------------
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
Re: Review Request 60933: Added stats/control helpers for the Blkio
cgroup subsystem.
Posted by Gilbert Song <so...@gmail.com>.
> 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
>
>
Re: Review Request 60933: Added stats/control helpers for the Blkio
cgroup subsystem.
Posted by Qian Zhang <zh...@gmail.com>.
> On July 18, 2017, 10:27 p.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?
>
> Gilbert Song wrote:
> Yes, so that we can use an object as a `dev_t` key in hashmap.
I see, thanks Gilbert!
> On July 18, 2017, 10:27 p.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.
>
> Gilbert Song wrote:
> A qualifier to distinguish `inline methods` to `static Try<Device>`. A style choice, seems more clear to me in this way.
+1
> On July 18, 2017, 10:27 p.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?
>
> Gilbert Song wrote:
> Two semantics:
> ```
> 8:0 Read 27660288
> Total 1000
> ```
Thanks Gilbert! Can you please put the possible semantics somewhere in the code as comments? It will be much easier for others to under the code logic:-)
> On July 18, 2017, 10:27 p.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.
>
> Gilbert Song wrote:
> https://github.com/apache/mesos/blob/1.3.0/src/linux/cgroups.cpp#L2572 can be fixed later.
Agree, mind to post a patch to fix that? :-)
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60933/#review180796
-----------------------------------------------------------
On July 20, 2017, 8:18 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> -----------------------------------------------------------
>
> (Updated July 20, 2017, 8:18 a.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
>
>
Re: Review Request 60933: Added stats/control helpers for the Blkio
cgroup subsystem.
Posted by Gilbert Song <so...@gmail.com>.
> 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.
>
> Gilbert Song wrote:
> https://github.com/apache/mesos/blob/1.3.0/src/linux/cgroups.cpp#L2572 can be fixed later.
>
> Qian Zhang wrote:
> Agree, mind to post a patch to fix that? :-)
Sure, will come up with a separte patch 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
>
>
Re: Review Request 60933: Added stats/control helpers for the Blkio
cgroup subsystem.
Posted by Gilbert Song <so...@gmail.com>.
> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 575 (patched)
> > <https://reviews.apache.org/r/60933/diff/1/?file=1778067#file1778067line575>
> >
> > 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)
Thought about that. Would prefer a corresponding `cfq` namespace.
> 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`.
yea, should be `unsigned int` here for major or minor.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60933/#review180796
-----------------------------------------------------------
On July 18, 2017, 2:39 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> -----------------------------------------------------------
>
> (Updated July 18, 2017, 2:39 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/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 60933: Added stats/control helpers for the Blkio
cgroup subsystem.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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
>
>