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/02 00:05:36 UTC
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 10:05 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Rebase and remove PID tests.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Update perf tests to including testing the supported perf output formats.
Diffs (updated)
-----
src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
Diff: https://reviews.apache.org/r/37466/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97366
-----------------------------------------------------------
Thanks for testing this!
src/tests/containerizer/perf_tests.cpp (line 67)
<https://reviews.apache.org/r/37466/#comment153253>
This one is never instantiated!
src/tests/containerizer/perf_tests.cpp (line 85)
<https://reviews.apache.org/r/37466/#comment153252>
All of these classes should include Perf in the name, otherwise these are not clear in the test results:
ParseCgroups/ParseCgroupsTest.ParseCgroups/0
How about:
Version/PerfCgroupsTest.Parse/0
src/tests/containerizer/perf_tests.cpp (lines 113 - 131)
<https://reviews.apache.org/r/37466/#comment153255>
To be consistent with the other code, please instantiate right after the class definition, rather than the test definition. i.e. move this up to right below the ParseCgroupsTest class.
Ditto for the others.
src/tests/containerizer/perf_tests.cpp (lines 153 - 164)
<https://reviews.apache.org/r/37466/#comment153256>
I don't think you need the comments here.
Also, shouldn't you do s/4.1.0/4.0.0/ here?
Ditto for the other instantiations.
- Ben Mahler
On Sept. 1, 2015, 10:05 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 10:05 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97348
-----------------------------------------------------------
Bad patch!
Reviews applied: [37416, 37442]
Failed command: ./support/apply-review.sh -n -r 37442
Error:
2015-09-01 22:18:04 URL:https://reviews.apache.org/r/37442/diff/raw/ [4003/4003] -> "37442.patch" [1]
error: patch failed: src/linux/perf.cpp:378
error: src/linux/perf.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Sept. 1, 2015, 10:05 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 10:05 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97530
-----------------------------------------------------------
Bad patch!
Reviews applied: [37416, 37442, 37462, 37466]
Failed command: ./support/apply-review.sh -n -r 37466
Error:
2015-09-02 20:58:54 URL:https://reviews.apache.org/r/37466/diff/raw/ [8757/8757] -> "37466.patch" [1]
error: patch failed: src/tests/containerizer/perf_tests.cpp:56
error: src/tests/containerizer/perf_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Sept. 2, 2015, 8:06 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 8:06 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Paul Brett <pa...@twopensource.com>.
> On Sept. 2, 2015, 10:47 p.m., Ben Mahler wrote:
> > src/tests/containerizer/perf_tests.cpp, line 67
> > <https://reviews.apache.org/r/37466/diff/7/?file=1062628#file1062628line67>
> >
> > Why is this called ParseTypes and the one below called ParseCgroups? They both seem to parse cgroup-based perf output, so I'm a bit confused.
Understood, now that we no longer support pid based tests, ParseTypes has become a strict subset of ParseCgroups and adds no value. I will delete it.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97551
-----------------------------------------------------------
On Sept. 2, 2015, 11:33 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 11:33 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97551
-----------------------------------------------------------
Thinking about this further, the test parameteratization here seems to be a bit of a mess, could we instead just create lists of valid / invalid inputs within a test and loop over them? I don't think the test parameterization here is worth the complexity.
src/tests/containerizer/perf_tests.cpp (line 51)
<https://reviews.apache.org/r/37466/#comment153537>
Why not just Events? This is testing both valid and invalid events.
Also, can you pull this out into a separate patch?
src/tests/containerizer/perf_tests.cpp (line 67)
<https://reviews.apache.org/r/37466/#comment153545>
Why is this called ParseTypes and the one below called ParseCgroups? They both seem to parse cgroup-based perf output, so I'm a bit confused.
src/tests/containerizer/perf_tests.cpp (lines 85 - 100)
<https://reviews.apache.org/r/37466/#comment153541>
Please instantiate right below the class as it makes it clearer when there are many tests under the test fixture (it's also what we do in all the other tests).
Ditto for the rest of these.
src/tests/containerizer/perf_tests.cpp (lines 223 - 233)
<https://reviews.apache.org/r/37466/#comment153544>
What's with the inconsistent newlines here?
src/tests/containerizer/perf_tests.cpp (line 253)
<https://reviews.apache.org/r/37466/#comment153543>
I think you meant to split the string literal on the newline like you did in most cases, can you do a sweep?
- Ben Mahler
On Sept. 2, 2015, 9:13 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 9:13 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97597
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37416, 37442, 37462, 37466]
All tests passed.
- Mesos ReviewBot
On Sept. 3, 2015, 12:43 a.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2015, 12:43 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/
-----------------------------------------------------------
(Updated Sept. 3, 2015, 12:43 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Reworked to eliminate value parameterized tests.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Update perf tests to including testing the supported perf output formats.
Diffs (updated)
-----
src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
Diff: https://reviews.apache.org/r/37466/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 11:33 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Incorporated review comments.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Update perf tests to including testing the supported perf output formats.
Diffs (updated)
-----
src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
Diff: https://reviews.apache.org/r/37466/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/#review97538
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37416, 37442, 37462, 37466]
All tests passed.
- Mesos ReviewBot
On Sept. 2, 2015, 9:13 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37466/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 9:13 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2834
> https://issues.apache.org/jira/browse/MESOS-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Update perf tests to including testing the supported perf output formats.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37466/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 9:13 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Rebase
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Update perf tests to including testing the supported perf output formats.
Diffs (updated)
-----
src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
Diff: https://reviews.apache.org/r/37466/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37466: Update perf tests to including testing the
supported perf output formats.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37466/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 8:06 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed reviewer comments. Reversed tuple order to improve readability. Remove ROOT flag from event validation test.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Update perf tests to including testing the supported perf output formats.
Diffs (updated)
-----
src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
Diff: https://reviews.apache.org/r/37466/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett