You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/02/13 20:11:50 UTC
Review Request 56611: Relax perf version check for Arch Linux.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/
-----------------------------------------------------------
Review request for mesos and Neil Conway.
Bugs: MESOS-6982
https://issues.apache.org/jira/browse/MESOS-6982
Repository: mesos
Description
-------
Relax perf version check for Arch Linux.
Diffs
-----
src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
Diff: https://reviews.apache.org/r/56611/diff/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by James Peach <jp...@apache.org>.
On Feb. 26, 2017, 7:48 p.m., James Peach wrote:
> > Sorry, one more thing to address and then I'll commit this: on certain versions of Ubuntu, `perf` is often installed but does not work correctly:
> >
> > ```
> > $ perf --version
> > WARNING: perf not found for kernel 3.19.0-28
> >
> > You may need to install the following packages for this specific kernel:
> > linux-tools-3.19.0-28-generic
> > linux-cloud-tools-3.19.0-28-generic
> >
> > You may also want to install one of the following packages to keep up to date:
> > linux-tools-generic
> > linux-cloud-tools-generic
> > ```
> >
> > On such a system, `PerfTest.Version` yields:
> >
> > ```
> > [ RUN ] PerfTest.Version
> > ../../mesos/src/tests/containerizer/perf_tests.cpp:134: Failure
> > (perf::version()).failure(): Failed to execute perf: exited with status 2
> > [ FAILED ] PerfTest.Version (51 ms)
> > [----------] 1 test from PerfTest (51 ms total)
> > ```
Rather than continuing to second guess packagers and cloud vendors, I added a check that the `perf --version` completes successfully (as opposed to perf merely being present). It feels a bit grungy but I expect it will be reasonably reliable for testing purposes.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review166819
-----------------------------------------------------------
On Feb. 14, 2017, 3:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review166819
-----------------------------------------------------------
src/linux/perf.cpp (line 212)
<https://reviews.apache.org/r/56611/#comment238895>
Should be `string`, not `std::string`.
Sorry, one more thing to address and then I'll commit this: on certain versions of Ubuntu, `perf` is often installed but does not work correctly:
```
$ perf --version
WARNING: perf not found for kernel 3.19.0-28
You may need to install the following packages for this specific kernel:
linux-tools-3.19.0-28-generic
linux-cloud-tools-3.19.0-28-generic
You may also want to install one of the following packages to keep up to date:
linux-tools-generic
linux-cloud-tools-generic
```
On such a system, `PerfTest.Version` yields:
```
[ RUN ] PerfTest.Version
../../mesos/src/tests/containerizer/perf_tests.cpp:134: Failure
(perf::version()).failure(): Failed to execute perf: exited with status 2
[ FAILED ] PerfTest.Version (51 ms)
[----------] 1 test from PerfTest (51 ms total)
```
- Neil Conway
On Feb. 14, 2017, 3:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review165983
-----------------------------------------------------------
Ship it!
Ship It!
- Neil Conway
On Feb. 14, 2017, 3:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review165532
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56611]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 14, 2017, 3:59 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 3:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review167132
-----------------------------------------------------------
Ship it!
Ship It!
- Neil Conway
On Feb. 28, 2017, 5:43 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 5:43 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review180843
-----------------------------------------------------------
src/tests/containerizer/perf_tests.cpp
Line 133 (original), 137-138 (patched)
<https://reviews.apache.org/r/56611/#comment256140>
Had a question about this comment when reviewing [r/60397](https://reviews.apache.org/r/60397/), this seems to imply the stub returns a non-zero exit status when run with `--version`? Is that true? It would be great to make that explicit in the comment:
```
// Note that on some systems, perf is a stub that asks you to
// install the right packages and returns a non-zero exit status.
```
- Benjamin Mahler
On Feb. 28, 2017, 5:43 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 5:43 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
>
> Diff: https://reviews.apache.org/r/56611/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review167083
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56611]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 28, 2017, 5:43 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 5:43 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 5:43 a.m.)
Review request for mesos and Neil Conway.
Changes
-------
Rebased and addresses review feedback.
Bugs: MESOS-6982
https://issues.apache.org/jira/browse/MESOS-6982
Repository: mesos
Description
-------
Relax perf version check for Arch Linux.
Diffs (updated)
-----
src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
Diff: https://reviews.apache.org/r/56611/diff/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 3:59 p.m.)
Review request for mesos and Neil Conway.
Changes
-------
Update tests.
Bugs: MESOS-6982
https://issues.apache.org/jira/browse/MESOS-6982
Repository: mesos
Description
-------
Relax perf version check for Arch Linux.
Diffs (updated)
-----
src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
Diff: https://reviews.apache.org/r/56611/diff/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review165455
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56611]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 14, 2017, 12:55 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 12:55 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 12:55 a.m.)
Review request for mesos and Neil Conway.
Changes
-------
Simplify and address review comments.
Bugs: MESOS-6982
https://issues.apache.org/jira/browse/MESOS-6982
Repository: mesos
Description
-------
Relax perf version check for Arch Linux.
Diffs (updated)
-----
src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
Diff: https://reviews.apache.org/r/56611/diff/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review165400
-----------------------------------------------------------
src/linux/perf.hpp (line 60)
<https://reviews.apache.org/r/56611/#comment237230>
This parameter should be named.
src/linux/perf.cpp (line 212)
<https://reviews.apache.org/r/56611/#comment237232>
Can we be more specific than just saying "lax manner"?
src/linux/perf.cpp (line 227)
<https://reviews.apache.org/r/56611/#comment237233>
Needs `#include <algorithm>` for `std::min`. Also, I think `size_t` would be more idiomatic than `unsigned`.
src/linux/perf.cpp (line 234)
<https://reviews.apache.org/r/56611/#comment237234>
The logic here seems a bit strained. Wouldn't it be simpler to just say: (a) we parse at most the first three components of the version, (b) return an error if either of the first two components can't be parsed and default to zero for third component.
src/linux/perf.cpp (line 235)
<https://reviews.apache.org/r/56611/#comment237231>
Whitespace before `+`, also after `:`.
- Neil Conway
On Feb. 13, 2017, 8:11 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 8:11 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56611: Relax perf version check for Arch Linux.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56611/#review165394
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56611]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 13, 2017, 8:11 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 8:11 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Relax perf version check for Arch Linux.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
> Diff: https://reviews.apache.org/r/56611/diff/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>