Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-31 Thread James Peach 

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


Ship it!




Ship It!

- James Peach


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

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-29 Thread Gilbert Song 

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

(Updated July 29, 2017, 4:59 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, James Peach, 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 (updated)
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


Diff: https://reviews.apache.org/r/60933/diff/6/

Changes: https://reviews.apache.org/r/60933/diff/5-6/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-28 Thread James Peach 

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




src/linux/cgroups.hpp
Lines 458 (patched)


I'd argue that this is a `Value`, not an entry. I see that the value is 
obtained from a file (entry) in the cgroup, but the important aspect to capture 
in the name is that it is a value, not where the value came from.



src/linux/cgroups.hpp
Lines 468 (patched)


Since this is only used in the parsing, make this a local static in the cpp 
file.



src/linux/cgroups.hpp
Lines 471 (patched)


Since this is only used in the parser, make this a local status in the cpp 
file.



src/linux/cgroups.cpp
Lines 1966 (patched)


Isn't this the same as:
```
return Entry{None(), None(), value.get()}";
```

Here and below.


- James Peach


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

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-27 Thread Gilbert Song 

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

(Updated July 27, 2017, 5:30 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 (updated)
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


Diff: https://reviews.apache.org/r/60933/diff/5/

Changes: https://reviews.apache.org/r/60933/diff/4-5/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-25 Thread Gilbert Song 

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

(Updated July 25, 2017, 6:02 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 (updated)
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


Diff: https://reviews.apache.org/r/60933/diff/4/

Changes: https://reviews.apache.org/r/60933/diff/3-4/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-25 Thread Gilbert Song 


> On July 20, 2017, 11:43 p.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 451 (patched)
> > 
> >
> > Just curious, this refers to the blkio file `blkio.weight` and 
> > `blkio.leaf_weight`, right?

Yes.


> On July 20, 2017, 11:43 p.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1969 (patched)
> > 
> >
> > We do not need to explicitly set it to `None()`, just `Option` 
> > should be OK.

I personally very like it to be explicit since it would remind people the fact 
that the device may not exist. But I decide to remove it for duplicate variable 
assignment.


- Gilbert


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


On July 20, 2017, 4:58 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> ---
> 
> (Updated July 20, 2017, 4:58 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-20 Thread Qian Zhang 

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


Fix it, then Ship it!





src/linux/cgroups.hpp
Lines 451 (patched)


Just curious, this refers to the blkio file `blkio.weight` and 
`blkio.leaf_weight`, right?



src/linux/cgroups.cpp
Lines 1969 (patched)


We do not need to explicitly set it to `None()`, just `Option` 
should be OK.


- Qian Zhang


On July 21, 2017, 7:58 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> ---
> 
> (Updated July 21, 2017, 7:58 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-20 Thread Qian Zhang 


> On July 20, 2017, 8:39 p.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1986 (patched)
> > 
> >
> > So this means there can be an entry without operation? But I think the 
> > possible semantics are:
> > ```
> > 8:0 Read 27660288
> > Total 1000
> > ```
> > So it seems not possible to have an entry without operation, instead it 
> > can be an entry without device number.
> 
> Gilbert Song wrote:
> So basically, for now we have:
> ```
> 8:0 Read 27660288
> Total 1000
> 7:0 100
> ```
> 
> Operations are required for those repeated fields in `blkio` protobuf. 
> Specifically `sectors` and `time` do not have an operation. Let me add some 
> comments here.

I see, thanks Gilbert!


- Qian


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


On July 21, 2017, 7:58 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> ---
> 
> (Updated July 21, 2017, 7:58 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-20 Thread Gilbert Song 

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

(Updated July 20, 2017, 4:58 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 (updated)
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


Diff: https://reviews.apache.org/r/60933/diff/3/

Changes: https://reviews.apache.org/r/60933/diff/2-3/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-20 Thread Gilbert Song 


> On July 20, 2017, 5:39 a.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1986 (patched)
> > 
> >
> > So this means there can be an entry without operation? But I think the 
> > possible semantics are:
> > ```
> > 8:0 Read 27660288
> > Total 1000
> > ```
> > So it seems not possible to have an entry without operation, instead it 
> > can be an entry without device number.

So basically, for now we have:
```
8:0 Read 27660288
Total 1000
7:0 100
```

Operations are required for those repeated fields in `blkio` protobuf. 
Specifically `sectors` and `time` do not have an operation. Let me add some 
comments here.


> On July 20, 2017, 5:39 a.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 2005-2009 (patched)
> > 
> >
> > I think it should be:
> > ```
> >   return (s == "Total" ||
> >   s == "Read" ||
> >   s == "Write" ||
> >   s == "Sync" ||
> >   s == "Async");
> > ```
> > See `mesos::internal::protobuf::isTerminalState()` as an example.

Thanks, Qian!


- Gilbert


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


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.

2017-07-20 Thread Qian Zhang 

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




src/linux/cgroups.hpp
Lines 582-583 (patched)


Remove `along with devices and types of operations`?



src/linux/cgroups.hpp
Lines 586-587 (original), 590-591 (patched)


Ditto.



src/linux/cgroups.cpp
Line 1994 (original), 1972 (patched)


Kill `stat`.



src/linux/cgroups.cpp
Lines 1986 (patched)


So this means there can be an entry without operation? But I think the 
possible semantics are:
```
8:0 Read 27660288
Total 1000
```
So it seems not possible to have an entry without operation, instead it can 
be an entry without device number.



src/linux/cgroups.cpp
Lines 2005-2009 (patched)


I think it should be:
```
  return (s == "Total" ||
  s == "Read" ||
  s == "Write" ||
  s == "Sync" ||
  s == "Async");
```
See `mesos::internal::protobuf::isTerminalState()` as an example.


- Qian Zhang


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.

2017-07-20 Thread Gilbert Song 


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1944 (patched)
> > 
> >
> > 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.

2017-07-20 Thread Qian Zhang 


> On July 18, 2017, 10:27 p.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 427 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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`. 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)
> > 
> >
> > 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)
> > 
> >
> > 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.

2017-07-19 Thread Gilbert Song 


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 427 (patched)
> > 
> >
> > 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)
> > 
> >
> > I think there is no need to have a second `public:` in this class.

A qualifier to distinguish `inline methods` to `static Try`. 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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>`?

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

2017-07-19 Thread Gilbert Song 

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


Changes
---

Addressed comments.


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 (updated)
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


Diff: https://reviews.apache.org/r/60933/diff/2/

Changes: https://reviews.apache.org/r/60933/diff/1-2/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

2017-07-19 Thread Gilbert Song 


> On July 18, 2017, 7:27 a.m., Qian Zhang wrote:
> > src/linux/cgroups.hpp
> > Lines 575 (patched)
> > 
> >
> > 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.)

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

2017-07-18 Thread Qian Zhang 

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




src/linux/cgroups.hpp
Lines 422 (patched)


Suggest to separte this to 3 lines:
```
{
  return value == that.value;
}
```



src/linux/cgroups.hpp
Lines 425 (patched)


Ditto.



src/linux/cgroups.hpp
Lines 427 (patched)


Why do we need this?



src/linux/cgroups.hpp
Lines 429 (patched)


I think there is no need to have a second `public:` in this class.



src/linux/cgroups.hpp
Lines 446-462 (patched)


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)


Why is this an optional field? Any chance there is no device in an entry?



src/linux/cgroups.hpp
Lines 488 (patched)


s/blkio requests/BIOS requests/



src/linux/cgroups.hpp
Lines 495 (patched)


Ditto.



src/linux/cgroups.hpp
Lines 545-546 (patched)


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



src/linux/cgroups.hpp
Lines 575 (patched)


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



src/linux/cgroups.hpp
Lines 575-576 (patched)


Merge into a single line.



src/linux/cgroups.cpp
Lines 1939 (patched)


Kill this empty line.



src/linux/cgroups.cpp
Lines 1944 (patched)


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)


Suggest to change this message to:
```
"Invalid device major number: '" + device[0] + "'"
```



src/linux/cgroups.cpp
Lines 1951 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2035 (patched)


Kill this empty line.



src/linux/cgroups.cpp
Lines 2044 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2063 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2072 (patched)


Ditto.



src/linux/cgroups.cpp
Lines 2261-2262 (patched)


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