Re: Review Request 37466: Update perf tests to including testing the supported perf output formats.

2015-09-02 Thread Paul Brett 


> On Sept. 2, 2015, 10:47 p.m., Ben Mahler wrote:
> > src/tests/containerizer/perf_tests.cpp, line 67
> > 
> >
> > 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.

2015-09-02 Thread Mesos ReviewBot 

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

2015-09-02 Thread Paul Brett 

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

2015-09-02 Thread Mesos ReviewBot 

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

2015-09-02 Thread Paul Brett 

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

2015-09-02 Thread Ben Mahler 

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


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)


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)


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)


What's with the inconsistent newlines here?



src/tests/containerizer/perf_tests.cpp (line 253)


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.

2015-09-01 Thread Ben Mahler 

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


This one is never instantiated!



src/tests/containerizer/perf_tests.cpp (line 85)


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)


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)


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.

2015-09-01 Thread Mesos ReviewBot 

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

2015-08-24 Thread Cong Wang 

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



src/tests/containerizer/perf_tests.cpp (line 52)
https://reviews.apache.org/r/37466/#comment151466

I think this should still be in perf.hpp even though it is just for testing.


- Cong Wang


On Aug. 21, 2015, 4:06 a.m., Paul Brett wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37466/
 ---
 
 (Updated Aug. 21, 2015, 4:06 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/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
   src/tests/containerizer/perf_tests.cpp 
 6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 
 
 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.

2015-08-20 Thread Paul Brett 

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

(Updated Aug. 21, 2015, 4:06 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

Fix broken patch.


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/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
  src/tests/containerizer/perf_tests.cpp 
6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 

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.

2015-08-20 Thread Paul Brett 

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

(Updated Aug. 20, 2015, 11 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rework to use 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 
6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 

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.

2015-08-20 Thread Paul Brett 

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

(Updated Aug. 20, 2015, 11:51 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Correct indentation.


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

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.

2015-08-14 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37423, 37424, 37417, 37416, 37442, 37460, 37462, 37466]

All tests passed.

- Mesos ReviewBot


On Aug. 14, 2015, 3:39 a.m., Paul Brett wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37466/
 ---
 
 (Updated Aug. 14, 2015, 3:39 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 
 6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 
 
 Diff: https://reviews.apache.org/r/37466/diff/
 
 
 Testing
 ---
 
 sudo make check
 
 
 Thanks,
 
 Paul Brett

Review Request 37466: Update perf tests to including testing the supported perf output formats.

2015-08-13 Thread Paul Brett 

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

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

Diff: https://reviews.apache.org/r/37466/diff/


Testing
---

sudo make check


Thanks,

Paul Brett