Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 14, 2015, 2:53 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- fixed rebase issue Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 10, 2015, 8:47 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- review comments @benm Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 10, 2015, 10:46 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- code review @benm Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91119 --- src/linux/cgroups.hpp (line 445) https://reviews.apache.org/r/36106/#comment144393 1 space after @return and before Some, no? src/linux/cgroups.hpp (lines 473 - 476) https://reviews.apache.org/r/36106/#comment144395 You seem to be using variable space identation for this block unlike the one for the 'cgroup()'. Our styleguide seems to hint to use fixed indentation for these doxygen headers. - Till Toenshoff On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91129 --- src/tests/cgroups_tests.cpp (line 1191) https://reviews.apache.org/r/36106/#comment144403 Could you please add one or two lines of description for this test as a comment above the test itself? - Till Toenshoff On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 438-447 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line438 What is the plan for introducing javadoc comments? cgroups.hpp seems like a reasonable candidate, but we should avoid inconsistent comment formatting within a file :( I'd propose commenting in the existing style, and following up by doing a javadoc formatting sweep across the file, if you're interested. Sound good? I was explicitly asked to do this way for now (see above reviews). I can remove it but will be conflicting with the other reviewer. On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 450-455 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line450 Why are we documenting the cpuacct.stat file format here? The caller should only cares about the user and system times, not the format of the underlying file :) I thought extra information about where that data come from will help. On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 458-466 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line458 Are these kinds of comments providing any value? Just for serving doxygen. Not sure what else I could have added. Sugestions are welcome. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91168 --- On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91168 --- src/linux/cgroups.hpp (lines 438 - 447) https://reviews.apache.org/r/36106/#comment144458 What is the plan for introducing javadoc comments? cgroups.hpp seems like a reasonable candidate, but we should avoid inconsistent comment formatting within a file :( I'd propose commenting in the existing style, and following up by doing a javadoc formatting sweep across the file, if you're interested. Sound good? src/linux/cgroups.hpp (lines 450 - 455) https://reviews.apache.org/r/36106/#comment144457 Why are we documenting the cpuacct.stat file format here? The caller should only cares about the user and system times, not the format of the underlying file :) src/linux/cgroups.hpp (lines 458 - 466) https://reviews.apache.org/r/36106/#comment144456 Are these kinds of comments providing any value? - Ben Mahler On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 443-472 https://reviews.apache.org/r/36106/diff/1/?file=997646#file997646line443 Thanks! (1) Do you mind updating my TODO on cgroups::stat() to reflect that cpuacct::stat is implemented? (2) Can we just make this a simple struct with two non-const fields and remove the parse method and getters? Keep it simple and small, if we want immutability we can just have a 'const Stat' rather than forcing it on the caller :) (3) Any reason not to use 'Duration' for these fields? Jojy Varghese wrote: 1) Absolutely I can. 2) I wanted to reflect the semantics of the stats call. When you make a stats call, the data you get is immutable. By forcing external const, it would imply that the value is immutable. 3) Duration is a period ( i think) and the values here are absolute values derived from ticks. Joris Van Remoortere wrote: Regarding 3): Duration indeed represents an amount of time, rather than a specific point in time. I believe this is what we intend to represent in this code as well, right? The comments for your functions suggest an amount of time rather than a point in time. Regarding 1): I am working on another patch that addresses the containerizer (docker) using this API. I will also change the cpushare code along with that patch. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 6, 2015, 6:20 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- post review comments from joris. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2014 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 I don't think we usually define a inline function like this anywhere else, so curious to see what others think. Personally I don't think it provides any additional benefits here. Jojy Varghese wrote: The advantage is that otherwise we will end up copy-paste code of parsing at two places(line 0 and line 1) Timothy Chen wrote: What I meant to compare with is having a another named function defined. Jojy Varghese wrote: Having an external function is useful if its being used outside this function. Anonymous functions are meant to solve this. Timothy Chen wrote: No one else chimed in then I guess it's fine, I don't have any particular setback around this but this is most likely the first introduction of this. I won't be suprised if some other committer jumps on this. After adding lambda support, we can finally do things like this (YAY!) See the `Feel free to use auto when naming a lambda expression:` section of the [style guide](http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/) Specifically, if the function is not mutating local state, but rather represents a functional transformation this is a nice use of a lambda. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 6, 2015, 6:20 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 6, 2015, 6:20 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 443-472 https://reviews.apache.org/r/36106/diff/1/?file=997646#file997646line443 Thanks! (1) Do you mind updating my TODO on cgroups::stat() to reflect that cpuacct::stat is implemented? (2) Can we just make this a simple struct with two non-const fields and remove the parse method and getters? Keep it simple and small, if we want immutability we can just have a 'const Stat' rather than forcing it on the caller :) (3) Any reason not to use 'Duration' for these fields? Jojy Varghese wrote: 1) Absolutely I can. 2) I wanted to reflect the semantics of the stats call. When you make a stats call, the data you get is immutable. By forcing external const, it would imply that the value is immutable. 3) Duration is a period ( i think) and the values here are absolute values derived from ticks. Regarding 3): Duration indeed represents an amount of time, rather than a specific point in time. I believe this is what we intend to represent in this code as well, right? The comments for your functions suggest an amount of time rather than a point in time. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 6, 2015, 6:20 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 6, 2015, 6:20 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36106/#review90581
---
Hey Jojy, great work so far! Thanks for writing up the JIRA first ;-)
Just doing a first review as I familiarize myself with the code.
I think the pattern and style feedback is valuable regardless! :-D
Regarding the debate around `Duration`: regardless of whether `Duration` is
'overkill', I think there is value in consistency for the sake of readability.
Many of our utility functions use / return `Duration`s and so a user of this
code will very likely be familiar with the types of things they can do with a
`Duration`.
src/linux/cgroups.cpp (lines 2006 - 2010)
https://reviews.apache.org/r/36106/#comment143679
Even though these thoughts are related, if the assignment statement doesn't
fit on a single line, we tend to leave a blank line between the assignment and
the if block.
```
Tryhashmapstring, uint64_t stats =
cgroups::stat(hierarchy, cgroup, cpuacct.stat);
if (!stats.isSome()) {
return Error(stats.error());
}
```
Same elsewhere in this patch.
Not yours: We can also kill the trailing space for the template
specialization :-)
src/linux/cgroups.cpp (line 2008)
https://reviews.apache.org/r/36106/#comment143686
`stats.isError()` instead of requiring the negation.
Here and elsewhere.
src/linux/cgroups.cpp (lines 2016 - 2019)
https://reviews.apache.org/r/36106/#comment143683
Can you add a comment here as to why it's ok to cache this value? (As in
this value is not dynamic)
What does it mean for this value to be `0`? Should we be aborting here?
If we're cache the value, should we only check it once? If so:
Maybe it's worth pulling this out into a static function within this module
so that we can use it elsewhere. This value seems generally usefull.
What do you think?
src/linux/cgroups.cpp (line 2029)
https://reviews.apache.org/r/36106/#comment143684
We leave a space after c-style casts
`(double) stats`
Here and elsehwere.
src/tests/cgroups_tests.cpp (line 1192)
https://reviews.apache.org/r/36106/#comment143688
We currently don't allow namespace aliasing. I understand this isn't well
documented, sorry about that!
src/tests/cgroups_tests.cpp (line 1194)
https://reviews.apache.org/r/36106/#comment143687
Can make this `const`.
Side note: This file has a significant amount of `std::string` and
`std::vector` and trailing spaces for template specializations.
Maybe we can do a clean-up pass after this patch lands :-)
- Joris Van Remoortere
On July 6, 2015, 6:20 p.m., Jojy Varghese wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36106/
---
(Updated July 6, 2015, 6:20 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and
Timothy Chen.
Bugs: MESOS-2961
https://issues.apache.org/jira/browse/MESOS-2961
Repository: mesos
Description
---
cgroups implementation does not have a cpuacct subsystem implementation as of
today. Adding the implementation for stat function.
Changes:
- added Stats class to encapsulate cpuacct.stat data
- added implementation for cpuacct::stats
- added unit tests
Jira: MESOS-2961
Diffs
-
src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94
src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00
src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a
Diff: https://reviews.apache.org/r/36106/diff/
Testing
---
make check
Thanks,
Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
src/linux/cgroups.cpp, lines 2014-2027
https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014
Any reason you're not just re-using cgroups::stat here? I'd suggest
just calling cgroups::stat for now, should simplify this and make it easier
for the reader. :)
Jojy Varghese wrote:
The only reason being that the way cpuacct creates Stat should be
encapsulated in the cpuacct::Stat. This is the same reason there is a parse
method in Stat. But I can change it to use cgroups::stat if absolutely
necessary.
Jojy Varghese wrote:
Also wanted to highlight the semantics of Stat class to make it clear
what the intent is. Stat can only be created from data read from the stats
file (cpuacct.stat). Thus creating Stat without the file data should not be
allowed. Once the Stat object is created, you can not mutate it.
Ben Mahler wrote:
Imagine this only returned the user time, then we would return a
Duration, yes? We would not be returning a `Stat` class with one const 'user'
member, with a const getter and a static parse method. That would be
overkill, right?
That's the reasoning I have for making this a plain struct. This is the
same situation as above, but we need to return more than one value (e.g.
`os::UTSInfo`, `os::Memory`, `os::Load`, etc.)
Jojy Varghese wrote:
If we were to just return a single value, I would have preferred it to be
a rvalue. Duration is a possiblity but not ideal in this situation as we need
just the ticks to seconds convertion which is not provided by Duration.
Structures like UTSInfo, Memory etc should be non-mutable as they represet a
snapshot of data IMHO.
Ben Mahler wrote:
Why would the caller care about the ticks conversion? The caller just
wants to know the amount of cpu time (i.e. duration) spent in userland, the
tick conversion is an implementation detail of reading the file format, which
the caller does not want to be burdened with.
I didn't understand your comment about returning an rvalue, the return
value at the callsite is an rvalue inherently until you assign it a name..
and the return value in the function could be an rvalue if the Duration is
constructed inline at the end.. something I'm missing here? Assuming that
you're referring to the function body, why would you care about returning an
rvalue when we have copy elision?
Back to the point, from the ideological perspective, I agree with you,
immutability can be a very nice functional programming principle. However,
the perspective I'm coming from is the experiential perspective, one of
having worked on and maintained this code base for a few years, and having a
keen interest in seeing things remain simple, straightforward, and
maintainable. I'd like all of us to be able to come in to any piece of code
and be able to read and understand it without unnecessary effort. What I've
seen from when we've tried to embrace immutable structs is that the hit to
simplicity and straightforwardness is too high. First, one adds 'const'
members, which requires a constructor. Then, we run into the difficulty of
having no default assignment and/or assignment operator. So, things are made
non-const, and to keep const semantics, one has to add const getters to
ensure the caller cannot modify the members. Now this is usually ~5x the
amount of code as we originally ha
d with a simple struct. The cognitive burden of understanding the immutable
'Stat' abstraction is higher because it is inherently more complicated and
requires more time to read:
```
struct Stat
{
Duration user;
Duration system;
}
// VS
class Stat
{
public:
// Sometimes this is private and there is a factory.
Stat(Duration user, Duration system)
: user_(user), system_(system) {}
// Default construction for use as values in STL maps, etc.
Stat() {}
Duration user() const
{
return user_;
}
Duration system() const
{
return system_;
}
private:
// Non-const for assignability / default construction.
Duration user_;
Duration system_;
}
```
So while there are indeed situations where immutability is nice (e.g.
state::Variable), the cognitive overhead the code reader faces with the more
complicated Stat class is not worth it, when we're dealing with simple
wrappers of data, IMHO :)
Duration would be an overkill when userTimeInSecs is what is the only thing
required. But i dont have any strong opininions about it. But when we say that
a good design principle is an overkill, then that is a grey area. Readbility is
a function of experience. Having strong design principles in the code is an
indication of robustness and
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 5, 2015, 6:30 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- review changes Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90407 --- Patch looks great! Reviews applied: [36106] All tests passed. - Mesos ReviewBot On July 5, 2015, 6:30 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 5, 2015, 6:30 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90387 --- Patch looks great! Reviews applied: [36106] All tests passed. - Mesos ReviewBot On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 8:09 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.cpp, lines 2014-2027 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 Any reason you're not just re-using cgroups::stat here? I'd suggest just calling cgroups::stat for now, should simplify this and make it easier for the reader. :) Jojy Varghese wrote: The only reason being that the way cpuacct creates Stat should be encapsulated in the cpuacct::Stat. This is the same reason there is a parse method in Stat. But I can change it to use cgroups::stat if absolutely necessary. Jojy Varghese wrote: Also wanted to highlight the semantics of Stat class to make it clear what the intent is. Stat can only be created from data read from the stats file (cpuacct.stat). Thus creating Stat without the file data should not be allowed. Once the Stat object is created, you can not mutate it. Ben Mahler wrote: Imagine this only returned the user time, then we would return a Duration, yes? We would not be returning a `Stat` class with one const 'user' member, with a const getter and a static parse method. That would be overkill, right? That's the reasoning I have for making this a plain struct. This is the same situation as above, but we need to return more than one value (e.g. `os::UTSInfo`, `os::Memory`, `os::Load`, etc.) If we were to just return a single value, I would have preferred it to be a rvalue. Duration is a possiblity but not ideal in this situation as we need just the ticks to seconds convertion which is not provided by Duration. Structures like UTSInfo, Memory etc should be non-mutable as they represet a snapshot of data IMHO. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 8:09 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
src/linux/cgroups.cpp, lines 2014-2027
https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014
Any reason you're not just re-using cgroups::stat here? I'd suggest
just calling cgroups::stat for now, should simplify this and make it easier
for the reader. :)
Jojy Varghese wrote:
The only reason being that the way cpuacct creates Stat should be
encapsulated in the cpuacct::Stat. This is the same reason there is a parse
method in Stat. But I can change it to use cgroups::stat if absolutely
necessary.
Jojy Varghese wrote:
Also wanted to highlight the semantics of Stat class to make it clear
what the intent is. Stat can only be created from data read from the stats
file (cpuacct.stat). Thus creating Stat without the file data should not be
allowed. Once the Stat object is created, you can not mutate it.
Ben Mahler wrote:
Imagine this only returned the user time, then we would return a
Duration, yes? We would not be returning a `Stat` class with one const 'user'
member, with a const getter and a static parse method. That would be
overkill, right?
That's the reasoning I have for making this a plain struct. This is the
same situation as above, but we need to return more than one value (e.g.
`os::UTSInfo`, `os::Memory`, `os::Load`, etc.)
Jojy Varghese wrote:
If we were to just return a single value, I would have preferred it to be
a rvalue. Duration is a possiblity but not ideal in this situation as we need
just the ticks to seconds convertion which is not provided by Duration.
Structures like UTSInfo, Memory etc should be non-mutable as they represet a
snapshot of data IMHO.
Why would the caller care about the ticks conversion? The caller just wants to
know the amount of cpu time (i.e. duration) spent in userland, the tick
conversion is an implementation detail of reading the file format, which the
caller does not want to be burdened with.
I didn't understand your comment about returning an rvalue, the return value at
the callsite is an rvalue inherently until you assign it a name.. and the
return value in the function could be an rvalue if the Duration is constructed
inline at the end.. something I'm missing here? Assuming that you're referring
to the function body, why would you care about returning an rvalue when we have
copy elision?
Back to the point, from the ideological perspective, I agree with you,
immutability can be a very nice functional programming principle. However, the
perspective I'm coming from is the experiential perspective, one of having
worked on and maintained this code base for a few years, and having a keen
interest in seeing things remain simple, straightforward, and maintainable. I'd
like all of us to be able to come in to any piece of code and be able to read
and understand it without unnecessary effort. What I've seen from when we've
tried to embrace immutable structs is that the hit to simplicity and
straightforwardness is too high. First, one adds 'const' members, which
requires a constructor. Then, we run into the difficulty of having no default
assignment and/or assignment operator. So, things are made non-const, and to
keep const semantics, one has to add const getters to ensure the caller cannot
modify the members. Now this is usually ~5x the amount of code as we originally
had with
a simple struct. The cognitive burden of understanding the immutable 'Stat'
abstraction is higher because it is inherently more complicated and requires
more time to read:
```
struct Stat
{
Duration user;
Duration system;
}
// VS
class Stat
{
public:
// Sometimes this is private and there is a factory.
Stat(Duration user, Duration system)
: user_(user), system_(system) {}
// Default construction for use as values in STL maps, etc.
Stat() {}
Duration user() const
{
return user_;
}
Duration system() const
{
return system_;
}
private:
// Non-const for assignability / default construction.
Duration user_;
Duration system_;
}
```
So while there are indeed situations where immutability is nice (e.g.
state::Variable), the cognitive overhead the code reader faces with the more
complicated Stat class is not worth it, when we're dealing with simple wrappers
of data, IMHO :)
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36106/#review90288
---
On July 2, 2015, 8:09 p.m., Jojy Varghese wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36106/
---
(Updated July 2, 2015, 8:09 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Joris
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.cpp, lines 2014-2027 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 Any reason you're not just re-using cgroups::stat here? I'd suggest just calling cgroups::stat for now, should simplify this and make it easier for the reader. :) Jojy Varghese wrote: The only reason being that the way cpuacct creates Stat should be encapsulated in the cpuacct::Stat. This is the same reason there is a parse method in Stat. But I can change it to use cgroups::stat if absolutely necessary. Also wanted to highlight the semantics of Stat class to make it clear what the intent is. Stat can only be created from data read from the stats file (cpuacct.stat). Thus creating Stat without the file data should not be allowed. Once the Stat object is created, you can not mutate it. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 8:09 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.cpp, lines 2014-2027 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 Any reason you're not just re-using cgroups::stat here? I'd suggest just calling cgroups::stat for now, should simplify this and make it easier for the reader. :) Jojy Varghese wrote: The only reason being that the way cpuacct creates Stat should be encapsulated in the cpuacct::Stat. This is the same reason there is a parse method in Stat. But I can change it to use cgroups::stat if absolutely necessary. Jojy Varghese wrote: Also wanted to highlight the semantics of Stat class to make it clear what the intent is. Stat can only be created from data read from the stats file (cpuacct.stat). Thus creating Stat without the file data should not be allowed. Once the Stat object is created, you can not mutate it. Imagine this only returned the user time, then we would return a Duration, yes? We would not be returning a `Stat` class with one const 'user' member, with a const getter and a static parse method. That would be overkill, right? That's the reasoning I have for making this a plain struct. This is the same situation as above, but we need to return more than one value (e.g. `os::UTSInfo`, `os::Memory`, `os::Load`, etc.) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 8:09 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- Are you planning to also update cpushare.cpp? src/linux/cgroups.hpp (lines 438 - 441) https://reviews.apache.org/r/36106/#comment143295 For next time, you could have split this into its own patch in a chain, since it is independent to the stat() implementation. src/linux/cgroups.hpp (lines 443 - 472) https://reviews.apache.org/r/36106/#comment143294 Thanks! (1) Do you mind updating my TODO on cgroups::stat() to reflect that cpuacct::stat is implemented? (2) Can we just make this a simple struct with two non-const fields and remove the parse method and getters? Keep it simple and small, if we want immutability we can just have a 'const Stat' rather than forcing it on the caller :) (3) Any reason not to use 'Duration' for these fields? src/linux/cgroups.hpp (lines 489 - 492) https://reviews.apache.org/r/36106/#comment143293 Whoops, this duplicates cpu::cfs_quota_us() above! src/linux/cgroups.cpp (lines 2014 - 2027) https://reviews.apache.org/r/36106/#comment143296 Any reason you're not just re-using cgroups::stat here? I'd suggest just calling cgroups::stat for now, should simplify this and make it easier for the reader. :) - Ben Mahler On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 8:09 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 443-472 https://reviews.apache.org/r/36106/diff/1/?file=997646#file997646line443 Thanks! (1) Do you mind updating my TODO on cgroups::stat() to reflect that cpuacct::stat is implemented? (2) Can we just make this a simple struct with two non-const fields and remove the parse method and getters? Keep it simple and small, if we want immutability we can just have a 'const Stat' rather than forcing it on the caller :) (3) Any reason not to use 'Duration' for these fields? 1) Absolutely I can. 2) I wanted to reflect the semantics of the stats call. When you make a stats call, the data you get is immutable. By forcing external const, it would imply that the value is immutable. 3) Duration is a period ( i think) and the values here are absolute values derived from ticks. On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 489-492 https://reviews.apache.org/r/36106/diff/1/?file=997646#file997646line489 Whoops, this duplicates cpu::cfs_quota_us() above! whoops! On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.cpp, lines 2014-2027 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 Any reason you're not just re-using cgroups::stat here? I'd suggest just calling cgroups::stat for now, should simplify this and make it easier for the reader. :) The only reason being that the way cpuacct creates Stat should be encapsulated in the cpuacct::Stat. This is the same reason there is a parse method in Stat. But I can change it to use cgroups::stat if absolutely necessary. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 8:09 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 6:01 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Changes --- review changes Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90268 --- Ship it! I'm not entirely sure the Doxygen comments fully comply with our coding style (spacing, indentation, enclosing linkable to classes in back-ticks ` `, etc.) but I'm not familiar enough with it to really comment. And this is in any case more documentation than 90% of our codebase, so... yay! :) - Marco Massenzio On July 2, 2015, 6:01 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 6:01 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2014 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 I don't think we usually define a inline function like this anywhere else, so curious to see what others think. Personally I don't think it provides any additional benefits here. Jojy Varghese wrote: The advantage is that otherwise we will end up copy-paste code of parsing at two places(line 0 and line 1) Timothy Chen wrote: What I meant to compare with is having a another named function defined. Jojy Varghese wrote: Having an external function is useful if its being used outside this function. Anonymous functions are meant to solve this. No one else chimed in then I guess it's fine, I don't have any particular setback around this but this is most likely the first introduction of this. I won't be suprised if some other committer jumps on this. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 2, 2015, 6:01 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 2, 2015, 6:01 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2014 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 I don't think we usually define a inline function like this anywhere else, so curious to see what others think. Personally I don't think it provides any additional benefits here. The advantage is that otherwise we will end up copy-paste code of parsing at two places(line 0 and line 1) On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2033 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2033 Space before Since they are logically related statements, i kept them without space. Spaces are logical seperators. On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2042 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2042 Space before Same as above. On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2060 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2060 Why add trailing underscore? As a member variable(is accepted according to mesos coding style guidelines) - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2060 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2060 Why add trailing underscore? Jojy Varghese wrote: As a member variable(is accepted according to mesos coding style guidelines) I don't see where in our mesos coding style guide specified this? And if you look in the code base we only put trailing spaces if it clashes with something else, but in this case we should prefer without. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description (updated) --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2014 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 I don't think we usually define a inline function like this anywhere else, so curious to see what others think. Personally I don't think it provides any additional benefits here. Jojy Varghese wrote: The advantage is that otherwise we will end up copy-paste code of parsing at two places(line 0 and line 1) What I meant to compare with is having a another named function defined. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90140 --- src/linux/cgroups.hpp (lines 438 - 440) https://reviews.apache.org/r/36106/#comment143125 can you please use the Javadoc notation instead (as documented in the style guide)? also, please make sure to document @param, @return src/linux/cgroups.hpp (line 452) https://reviews.apache.org/r/36106/#comment143127 does the string need the trailing space before the newline? src/linux/cgroups.hpp (line 479) https://reviews.apache.org/r/36106/#comment143128 unnecessary blank line src/linux/cgroups.cpp (line 2002) https://reviews.apache.org/r/36106/#comment143130 and, in any event, the variables should have a meaningful name - should these be `userTime` or something? (also, if they are time durations, please pre-pend the unit: `userTimeSec`) - Marco Massenzio On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/tests/cgroups_tests.cpp, line 1192 https://reviews.apache.org/r/36106/diff/1/?file=997648#file997648line1192 Add using to the top of the tests Since this is an alias (typedef) and scoped for the function. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/tests/cgroups_tests.cpp, line 1192 https://reviews.apache.org/r/36106/diff/1/?file=997648#file997648line1192 Add using to the top of the tests Jojy Varghese wrote: Since this is an alias (typedef) and scoped for the function. LoL - this is the same clash I had in my first review :) (not only that, but I think you're also going to have trouble: AFAIK alias are not allowed in the Mesos codebase - not sure why) Essentially, I objected to placing something used **only once** 1,190 (or so) lines above its point of usage - eventually lost that particular argument when the whole review was discarded... Good luck with getting this past our reviewers :) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:54 p.m., Marco Massenzio wrote: src/linux/cgroups.cpp, line 2002 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2002 and, in any event, the variables should have a meaningful name - should these be `userTime` or something? (also, if they are time durations, please pre-pend the unit: `userTimeSec`) Jojy Varghese wrote: Do you mean the argument names or member variable names? sorry - that was a 'malformed' comment... I meant `ut` and `st`: do they mean `userTimeSec`, `systemTimeSec`? Ideally, you would name both args and members the same (if you take a copy) but the private members with a trailing underscore: ``` userTimeSec_ = userTimeSec; ``` and I also actually meant with the time-unit **suffix** (not *pre-pend*), sorry. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90140 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:54 p.m., Marco Massenzio wrote: src/linux/cgroups.cpp, line 2002 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2002 and, in any event, the variables should have a meaningful name - should these be `userTime` or something? (also, if they are time durations, please pre-pend the unit: `userTimeSec`) Do you mean the argument names or member variable names? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90140 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2014 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014 I don't think we usually define a inline function like this anywhere else, so curious to see what others think. Personally I don't think it provides any additional benefits here. Jojy Varghese wrote: The advantage is that otherwise we will end up copy-paste code of parsing at two places(line 0 and line 1) Timothy Chen wrote: What I meant to compare with is having a another named function defined. Having an external function is useful if its being used outside this function. Anonymous functions are meant to solve this. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2060 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2060 Why add trailing underscore? Jojy Varghese wrote: As a member variable(is accepted according to mesos coding style guidelines) Timothy Chen wrote: I don't see where in our mesos coding style guide specified this? And if you look in the code base we only put trailing spaces if it clashes with something else, but in this case we should prefer without. In style guide (Variable naming section): Prefer trailing underscores for use as member fields (but not required). - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90174 --- Patch looks great! Reviews applied: [36106] All tests passed. - Mesos ReviewBot On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 1, 2015, 9:46 p.m., Timothy Chen wrote: src/linux/cgroups.cpp, line 2060 https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2060 Why add trailing underscore? Jojy Varghese wrote: As a member variable(is accepted according to mesos coding style guidelines) Timothy Chen wrote: I don't see where in our mesos coding style guide specified this? And if you look in the code base we only put trailing spaces if it clashes with something else, but in this case we should prefer without. Jojy Varghese wrote: In style guide (Variable naming section): Prefer trailing underscores for use as member fields (but not required). Hey Tim - this is a recent change, we discussed during a code review meeting. We now follow more closely the Google Style, by adding the trailing underscore on private variables. Check out the `FlagsBase` for an example of this. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90139 --- On July 1, 2015, 9:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 1, 2015, 9:38 p.m.) Review request for mesos, Ian Downes, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
