You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/09/01 01:57:29 UTC
Review Request 37976: Simplify perf by removing unused sampling
capabilities.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37976/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-3347
https://issues.apache.org/jira/browse/MESOS-3347
Repository: mesos
Description
-------
Simplify perf by removing unused sampling capabilities.
Diffs
-----
src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
src/linux/perf.cpp 56ef3917ff2d5f761bda7721f8fccec4a3e14b7c
src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6
src/tests/containerizer/perf_tests.cpp 6b3d70f3e7ea8f59f94e6961491d4e9a730e3334
Diff: https://reviews.apache.org/r/37976/diff/
Testing
-------
Thanks,
Paul Brett
Re: Review Request 37976: Simplify perf by removing unused sampling
capabilities.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37976/#review97230
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37976]
All tests passed.
- Mesos ReviewBot
On Aug. 31, 2015, 11:57 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37976/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2015, 11:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-3347
> https://issues.apache.org/jira/browse/MESOS-3347
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Simplify perf by removing unused sampling capabilities.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 56ef3917ff2d5f761bda7721f8fccec4a3e14b7c
> src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6
> src/tests/containerizer/perf_tests.cpp 6b3d70f3e7ea8f59f94e6961491d4e9a730e3334
>
> Diff: https://reviews.apache.org/r/37976/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37976: Simplify perf by removing unused sampling
capabilities.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37976/#review97218
-----------------------------------------------------------
Ship it!
Looking through git blame and https://issues.apache.org/jira/browse/MESOS-1278, this looks safe to remove to me.
The std:: removal could have been an independent patch (would have made it easier to review, and it lowers the surface area of the change which helps for blaming/reverting). I'll do this for you, but only this time since it's tedious on the reviewer and reviewing is our bottleneck. :)
I've also pulled out cleanups around using the -> operator.
src/linux/perf.cpp (lines 66 - 101)
<https://reviews.apache.org/r/37976/#comment153051>
No need for this to be pulled out now that there's only one code path.
src/linux/perf.cpp (lines 300 - 306)
<https://reviews.apache.org/r/37976/#comment153011>
Don't need this anymore.
src/linux/perf.cpp (lines 309 - 336)
<https://reviews.apache.org/r/37976/#comment153045>
No longer a need for the internal sample.
src/tests/containerizer/cgroups_tests.cpp (lines 1001 - 1002)
<https://reviews.apache.org/r/37976/#comment153014>
You can use an initializer list directly at the call instead of pulling out the variable.
src/tests/containerizer/cgroups_tests.cpp (lines 1007 - 1019)
<https://reviews.apache.org/r/37976/#comment153022>
Let's use the -> operator on Future now that we have it! :)
src/tests/containerizer/cgroups_tests.cpp (line 1008)
<https://reviews.apache.org/r/37976/#comment153021>
Why did you change this to 4 spaces? Wrapping after = is just 2 spaces.
src/tests/containerizer/cgroups_tests.cpp (line 1011)
<https://reviews.apache.org/r/37976/#comment153015>
We should also check that it contains the key before calling .at
src/tests/containerizer/perf_tests.cpp (line 27)
<https://reviews.apache.org/r/37976/#comment153050>
Don't need this anymore :)
src/tests/containerizer/perf_tests.cpp (line 63)
<https://reviews.apache.org/r/37976/#comment153016>
And here you re-added the std:: prefix? It would be really helpful if you could catch these through self-review :)
src/tests/containerizer/perf_tests.cpp (line 72)
<https://reviews.apache.org/r/37976/#comment153048>
Let's use the -> operator to clean this up, now that we have it
src/tests/containerizer/perf_tests.cpp (lines 91 - 92)
<https://reviews.apache.org/r/37976/#comment153047>
This fails the test!
src/tests/containerizer/perf_tests.cpp (line 97)
<https://reviews.apache.org/r/37976/#comment153049>
s/./,/
src/tests/containerizer/perf_tests.cpp (lines 100 - 101)
<https://reviews.apache.org/r/37976/#comment153046>
This fails the test!
- Ben Mahler
On Aug. 31, 2015, 11:57 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37976/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2015, 11:57 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-3347
> https://issues.apache.org/jira/browse/MESOS-3347
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Simplify perf by removing unused sampling capabilities.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 56ef3917ff2d5f761bda7721f8fccec4a3e14b7c
> src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6
> src/tests/containerizer/perf_tests.cpp 6b3d70f3e7ea8f59f94e6961491d4e9a730e3334
>
> Diff: https://reviews.apache.org/r/37976/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Paul Brett
>
>