You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/07/01 23:33:55 UTC
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
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
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
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
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
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
> 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
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
>
>
Re: Review Request 36106: cgroups: added cpuacct subsystem
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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
Posted by Joris Van Remoortere <jo...@gmail.com>.
> 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
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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
Posted by Timothy Chen <tn...@apache.org>.
> 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
Posted by Timothy Chen <tn...@apache.org>.
> 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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36106/#review90139
-----------------------------------------------------------
src/linux/cgroups.cpp (line 2002)
<https://reviews.apache.org/r/36106/#comment143124>
We usually name the constructor params and the variables the same.
src/linux/cgroups.cpp (line 2014)
<https://reviews.apache.org/r/36106/#comment143120>
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.
src/linux/cgroups.cpp (line 2033)
<https://reviews.apache.org/r/36106/#comment143117>
Space before
src/linux/cgroups.cpp (line 2042)
<https://reviews.apache.org/r/36106/#comment143118>
Space before
src/linux/cgroups.cpp (line 2053)
<https://reviews.apache.org/r/36106/#comment143115>
Add space before and after division operator.
src/linux/cgroups.cpp (line 2060)
<https://reviews.apache.org/r/36106/#comment143116>
Why add trailing underscore?
src/tests/cgroups_tests.cpp (line 1192)
<https://reviews.apache.org/r/36106/#comment143122>
Add using to the top of the tests
src/tests/cgroups_tests.cpp (line 1229)
<https://reviews.apache.org/r/36106/#comment143123>
Space before and after division
src/tests/cgroups_tests.cpp (line 1246)
<https://reviews.apache.org/r/36106/#comment143121>
Remove space
- Timothy Chen
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
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
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