subject:"Review Request 36106\: cgroups\: added cpuacct subsystem"

Re: Review Request 36106: cgroups: added cpuacct subsystem

2015-07-14 Thread Jojy Varghese


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

(Updated July 14, 2015, 5:05 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy 
Chen.


Changes
---

reveretd back to using std namespace


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

2015-07-13 Thread Jojy Varghese


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

2015-07-13 Thread Jojy Varghese


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

(Updated July 14, 2015, 12:38 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy 
Chen.


Changes
---

review comments @tim


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

  3rdparty/libprocess/3rdparty/Makefile.am 
856c2b289451fd404b97285b825e72913feb2f04 
  3rdparty/libprocess/3rdparty/stout/Makefile.am 
89e7b1854bd7f449f4f0027d76c6430d259a24de 
  3rdparty/libprocess/3rdparty/stout/configure.ac 
a1f86d0f943bbff91a8b021046eb66d624df7896 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
2394b95462182273464f0847f416ad83c3b64485 
  3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 
81f6e50baca95ad7a3a0ac1434c8db1c4de6adf2 
  3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
22d30eb80b4eca8be9cd4d48288f63fd52040ddd 
  3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp 
672820ab775648e459f34b0c0a20e4482cebfdac 
  3rdparty/libprocess/3rdparty/stout/tests/bytes_tests.cpp 
5239805704cce8a545e11fcd83aa80484617f582 
  3rdparty/libprocess/3rdparty/stout/tests/cache_tests.cpp 
6e42c7d56bd6ef47a968bee2a01ac47fb8a7ed26 
  3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 
ef2f87781e7abcf1d25bc7a12cb6e18f695b 
  3rdparty/libprocess/3rdparty/stout/tests/error_tests.cpp 
9e7605c53e6636e7fea32e4f69fbaff9100a979f 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
ebf8cd656625b7fd414cacaa87f156c95df29438 
  3rdparty/libprocess/3rdparty/stout/tests/gzip_tests.cpp 
01425d7205032280987021adcc97eb862a80c42c 
  3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp 
984c02c982dc4dd8eb528d79e715847ef7a693b8 
  3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp 
2826742187411e5be8da0517859ec0d558fc2921 
  3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp 
134ea2ee675aba13ddb7a28545b25b4f3701d5d9 
  3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
532d064dcc5e31d2824df750b7d3e6bddabddbb9 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
dd692094e164e33ba0f13e2b994c14cbfd827cbf 
  3rdparty/libprocess/3rdparty/stout/tests/linkedhashmap_tests.cpp 
5e385f4e7c69dc252bdee2436c6a57f1a0f17d32 
  3rdparty/libprocess/3rdparty/stout/tests/mac_tests.cpp 
0d398069ecd76002d5ee61765f0dd77f3b40bbb4 
  3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
1410cf18132fbaddea1498736ac539f77232de38 
  3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
b625ffaeb3672f58fbd9558a868f87404e659c53 
  3rdparty/libprocess/3rdparty/stout/tests/none_tests.cpp 
55e75207a8c5d2ac17cbee848855f4c66a2d2eb4 
  3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp 
0c3f89bafe1afb15d1a2d775ed598cdf1a5ea147 
  3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp 
e740d5bc0f0cc5cf8e99b2064c1e39c08282da67 
  3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 
de86232cd4a646c63cdb41d18cd4375615cb42e4 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
2556bd428cc8990659e30e804b9c96c1659ef4a1 
  3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp 
5d24f21f63433b8525370736dd630880d324ebeb 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
018ff51d10f4ba076609704d6e3b2c704c82b016 
  3rdparty/libprocess/3rdparty/stout/tests/set_tests.cpp 
6f6b0babf896216815f447b2548b4a8036751d22 
  3rdparty/libprocess/3rdparty/stout/tests/some_tests.cpp 
efb94ef3523e85d08498b5a3d5744d387ac5 
  3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 
ffe5f1bcf764f24cec1d191dcde289976b6281c4 
  3rdparty/libprocess/3rdparty/stout/tests/subcommand_tests.cpp 
f0a69d49c7c97b2d437c751fbee0a9c0f0ada0a9 
  3rdparty/libprocess/3rdparty/stout/tests/thread_tests.cpp 
319fcdf517b24f5bb9c85dad4093b09ec87e915e 
  3rdparty/libprocess/3rdparty/stout/tests/uuid_tests.cpp 
b931151ed62d2a356c9c96188ae967089d92168c 
  3rdparty/libprocess/Makefile.am e9c42d7ecd44eebeadcc3d1d5011e01674b3415a 
  3rdparty/libprocess/configure.ac 7d1221bd5ddfc4fa816b0bbea0be5c6b2cbb 
  3rdparty/libprocess/examples/example.cpp 
22ae083d8b5bf59cf52e18405e005cfe94edb81d 
  3rdparty/libprocess/include/Makefile.am 
3b6108dd37d23bd5104162e9e8f4a3aa0518cdcd 
  3rdparty/libprocess/include/process/address.hpp 
be216db823160f5db1dfb4502bf832246fb3df6d 
  3rdparty/libprocess/include/process/async.hpp 
d0c560aa48ef1c88407a6b1c42223fce3170245c 
  3rdparty/libprocess/include/process/clock.hpp 
0f73f894c753711db4fdefa9df40d5674aacc6f7 
  3rdparty/libprocess/include/process/

Re: Review Request 36106: cgroups: added cpuacct subsystem

2015-07-13 Thread Timothy Chen


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

Ship it!


Ship It!


src/linux/cgroups.cpp (line 2022)


Extra space in the message.



src/tests/cgroups_tests.cpp (line 1193)


We should use string instead of std::string here but I can fix that



src/tests/cgroups_tests.cpp (line 1193)


We should use string instead of std::string here but I can fix that


- Timothy Chen


On July 10, 2015, 10:46 p.m., Jojy Varghese wrote:
> 
> ---
> 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.
> 
> 
> 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

2015-07-10 Thread Jojy Varghese


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

2015-07-10 Thread Timothy Chen


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



src/linux/cgroups.hpp (line 438)


benm: Remove 1 space.



src/linux/cgroups.cpp (line 2003)


benm: s/std::string/string/g



src/linux/cgroups.cpp (line 2004)


ditto



src/linux/cgroups.cpp (line 2022)


benm: s/Error getting/Failed to get/g

Also we should use ErrnoError as well.



src/linux/cgroups.cpp (line 2030)


benm: All the error messages in this file follows the "Failed to..." 
instead of "Error..." message, we should be consistent.



src/tests/cgroups_tests.cpp (line 62)


benm: Remove alias



src/tests/cgroups_tests.cpp (line 1199)


benm: Remove storing result and just 
ASSERT_SOME(cgroups::cpuacct::stat(hierarchy, TEST_CGROUPS_ROOT));



src/tests/cgroups_tests.cpp (line 1203)


benm: AWAIT_READY(cgroups::destroy(hierarchy, TEST_CGROUPS_ROOT));


- Timothy Chen


On July 10, 2015, 8:47 p.m., Jojy Varghese wrote:
> 
> ---
> 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.
> 
> 
> 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

2015-07-10 Thread Jojy Varghese


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

2015-07-09 Thread Jojy Varghese


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

(Updated July 10, 2015, 1:06 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy 
Chen.


Changes
---

review comments @till


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

2015-07-09 Thread Jojy Varghese



> On July 9, 2015, 7:36 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 438-447
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

2015-07-09 Thread Ben Mahler


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



src/linux/cgroups.hpp (lines 438 - 447)


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)


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)


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

2015-07-09 Thread Till Toenshoff


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



src/tests/cgroups_tests.cpp (line 1191)


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

2015-07-09 Thread Till Toenshoff


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



src/linux/cgroups.hpp (line 445)


1 space after @return and before "Some", no?



src/linux/cgroups.hpp (lines 473 - 476)


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

2015-07-07 Thread Jojy Varghese



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 443-472
> > 
> >
> > 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

2015-07-06 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [36106]

All tests passed.

- Mesos ReviewBot


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

2015-07-06 Thread Jojy Varghese


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

2015-07-06 Thread Joris Van Remoortere



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 443-472
> > 
> >
> > 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

2015-07-06 Thread Joris Van Remoortere

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

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.

```
Try > 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)

`stats.isError()` instead of requiring the negation.

Here and elsewhere.

src/linux/cgroups.cpp (lines 2016 - 2019)

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)

We leave a space after c-style casts
`(double) stats`

Here and elsehwere.

src/tests/cgroups_tests.cpp (line 1192)

We currently don't allow namespace aliasing. I understand this isn't well 
documented, sorry about that!

src/tests/cgroups_tests.cpp (line 1194)

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

2015-07-06 Thread Joris Van Remoortere



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2014
> > 
> >
> > 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

2015-07-06 Thread Jojy Varghese


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

2015-07-05 Thread Mesos ReviewBot


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

2015-07-05 Thread Jojy Varghese


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

2015-07-05 Thread Jojy Varghese



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > 
> >
> > 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. Readbili

Re: Review Request 36106: cgroups: added cpuacct subsystem

2015-07-04 Thread Mesos ReviewBot


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

2015-07-03 Thread Ben Mahler

> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > 
> >
> > 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.)
> 
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

2015-07-03 Thread Jojy Varghese



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > 
> >
> > 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

2015-07-02 Thread Ben Mahler



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > 
> >
> > 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

2015-07-02 Thread Jojy Varghese



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 2014-2027
> > 
> >
> > 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

2015-07-02 Thread Jojy Varghese



> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 443-472
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

2015-07-02 Thread Ben Mahler

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

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)

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)

Whoops, this duplicates cpu::cfs_quota_us() above!

src/linux/cgroups.cpp (lines 2014 - 2027)

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

2015-07-02 Thread Timothy Chen



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2014
> > 
> >
> > 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

2015-07-02 Thread Marco Massenzio


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

2015-07-02 Thread Jojy Varghese


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

2015-07-01 Thread Marco Massenzio



> On July 1, 2015, 9:54 p.m., Marco Massenzio wrote:
> > src/linux/cgroups.cpp, line 2002
> > 
> >
> > 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

2015-07-01 Thread Marco Massenzio



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/tests/cgroups_tests.cpp, line 1192
> > 
> >
> > 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

2015-07-01 Thread Marco Massenzio



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2060
> > 
> >
> > 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

2015-07-01 Thread Mesos ReviewBot


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

2015-07-01 Thread Jojy Varghese



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2060
> > 
> >
> > 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

2015-07-01 Thread Jojy Varghese



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2014
> > 
> >
> > 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

2015-07-01 Thread Jojy Varghese



> On July 1, 2015, 9:54 p.m., Marco Massenzio wrote:
> > src/linux/cgroups.cpp, line 2002
> > 
> >
> > 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

2015-07-01 Thread Timothy Chen



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2060
> > 
> >
> > 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

2015-07-01 Thread Timothy Chen



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2014
> > 
> >
> > 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

2015-07-01 Thread Marco Massenzio


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



src/linux/cgroups.hpp (lines 438 - 440)


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)


does the string need the trailing space before the newline?



src/linux/cgroups.hpp (line 479)


unnecessary blank line



src/linux/cgroups.cpp (line 2002)


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

2015-07-01 Thread Jojy Varghese



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/tests/cgroups_tests.cpp, line 1192
> > 
> >
> > 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

2015-07-01 Thread Jojy Varghese



> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2014
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Space before

Same as above.


> On July 1, 2015, 9:46 p.m., Timothy Chen wrote:
> > src/linux/cgroups.cpp, line 2060
> > 
> >
> > 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

2015-07-01 Thread Timothy Chen


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



src/linux/cgroups.cpp (line 2002)


We usually name the constructor params and the variables the same.



src/linux/cgroups.cpp (line 2014)


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)


Space before



src/linux/cgroups.cpp (line 2042)


Space before



src/linux/cgroups.cpp (line 2053)


Add space before and after division operator.



src/linux/cgroups.cpp (line 2060)


Why add trailing underscore?



src/tests/cgroups_tests.cpp (line 1192)


Add using to the top of the tests



src/tests/cgroups_tests.cpp (line 1229)


Space before and after division



src/tests/cgroups_tests.cpp (line 1246)


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

2015-07-01 Thread Jojy Varghese


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

2015-07-01 Thread Jojy Varghese


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

47 matches

Mail list logo