Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 31, 2015, 7:41 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 31, 2015, 10:14 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 31, 2015, 10:27 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 31, 2015, 10:27 p.m.) Review request for mesos and Ben Mahler. Changes --- Rebased Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review96166 --- src/linux/perf.cpp (line 418) https://reviews.apache.org/r/37416/#comment151460 This is not expected, right? src/linux/perf.cpp (line 477) https://reviews.apache.org/r/37416/#comment151461 Make it complete, Perf version is not available. - Cong Wang On Aug. 21, 2015, 6:46 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 21, 2015, 6:46 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review96172 --- src/linux/perf.cpp (line 418) https://reviews.apache.org/r/37416/#comment151479 It would be a rare event but not completly unexpected. After all, perf can be upgraded while mesos is running. - Paul Brett On Aug. 21, 2015, 6:46 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 21, 2015, 6:46 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
On Aug. 24, 2015, 5:53 p.m., Cong Wang wrote: src/linux/perf.cpp, line 418 https://reviews.apache.org/r/37416/diff/5/?file=1045148#file1045148line418 This is not expected, right? Paul Brett wrote: It would be a rare event but not completly unexpected. After all, perf can be upgraded while mesos is running. I meant your s/is/it/ change is not expected... - Cong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review96166 --- On Aug. 21, 2015, 6:46 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 21, 2015, 6:46 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
On Aug. 24, 2015, 5:53 p.m., Cong Wang wrote: src/linux/perf.cpp, line 418 https://reviews.apache.org/r/37416/diff/5/?file=1045148#file1045148line418 This is not expected, right? It would be a rare event but not completly unexpected. After all, perf can be upgraded while mesos is running. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review96166 --- On Aug. 21, 2015, 6:46 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 21, 2015, 6:46 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 21, 2015, 6:46 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
On Aug. 19, 2015, 1:42 a.m., Ben Mahler wrote: src/linux/perf.cpp, lines 474-478 https://reviews.apache.org/r/37416/diff/3/?file=1043975#file1043975line474 Why `_supported` here that takes a version? Why not just have supported compute the version and then perform the necessary check? We need two variants - perf::supported that computes the version and performs the necessary checks, and a version for testing that we can feed fake version information into so that we can test the parsing against multiple versions of perf output. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review95809 --- On Aug. 20, 2015, 1:13 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 20, 2015, 1:13 a.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 20, 2015, 4:39 p.m.) Review request for mesos and Ben Mahler. Changes --- Fix bad patch. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
On Aug. 19, 2015, 1:42 a.m., Ben Mahler wrote:
src/linux/perf.cpp, lines 484-490
https://reviews.apache.org/r/37416/diff/3/?file=1043975#file1043975line484
Couple of things:
(1) Let's add a comment as to why we're using await here, since it is
an anti-pattern. Namely, is it because making supported() asynchronous is a
difficult change?
(2) Let's discard the future when it doesn't transition in the timeout.
(3) We're going to silently say false if the future fails, can we log
the failure?
(4) 'if (' :)
(5) Can you add some newlines here to make it less dense?
What is the advantage of calling discard() on version here? At the moment, we
pass version as a const ref, so calling discard is not possible. We could
remove the const constraint, but then we would have to pass an lvalue from
supported.
- Paul
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review95809
---
On Aug. 20, 2015, 4:39 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/
---
(Updated Aug. 20, 2015, 4:39 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
---
Perf supported() should be based on the version of perf, not the version of
the kernel.
Diffs
-
src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78
Diff: https://reviews.apache.org/r/37416/diff/
Testing
---
sudo make check
Thanks,
Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 19, 2015, 10:18 p.m.) Review request for mesos and Ben Mahler. Changes --- Remove dependency. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 20, 2015, 1:13 a.m.) Review request for mesos and Ben Mahler. Changes --- Incorporate review comments. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review95779 --- Just some notes before you rebase. src/linux/perf.cpp (lines 411 - 413) https://reviews.apache.org/r/37416/#comment150896 How about just saying that this returns the version of perf? :) src/linux/perf.cpp (line 415) https://reviews.apache.org/r/37416/#comment150898 whoops? src/linux/perf.cpp (line 420) https://reviews.apache.org/r/37416/#comment150897 output can just be a string since this is a .then continuation, no need to wrap into a Future src/linux/perf.cpp (lines 422 - 426) https://reviews.apache.org/r/37416/#comment150895 strings::remove w/ PREFIX should be easier to use than find / erase here src/linux/perf.cpp (line 516) https://reviews.apache.org/r/37416/#comment150899 Remember to use a space here - Ben Mahler On Aug. 13, 2015, 6:40 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 13, 2015, 6:40 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 19, 2015, 12:57 a.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review95809
---
src/linux/perf.cpp (lines 384 - 385)
https://reviews.apache.org/r/37416/#comment150952
Any reason to put this in internal::? It seems nice to have this as
perf::version, even though it's only in the cpp file.
src/linux/perf.cpp (line 390)
https://reviews.apache.org/r/37416/#comment150950
const string
how about a newline for readability?
src/linux/perf.cpp (line 391)
https://reviews.apache.org/r/37416/#comment150951
End comments with a period please :)
src/linux/perf.cpp (lines 474 - 478)
https://reviews.apache.org/r/37416/#comment150953
Why `_supported` here that takes a version? Why not just have supported
compute the version and then perform the necessary check?
src/linux/perf.cpp (lines 480 - 481)
https://reviews.apache.org/r/37416/#comment150954
Perf versions are numbered similarly to linux versions..?
Not yours but mind ending it with a period?
src/linux/perf.cpp (lines 483 - 489)
https://reviews.apache.org/r/37416/#comment150955
Couple of things:
(1) Let's add a comment as to why we're using await here, since it is an
anti-pattern. Namely, is it because making supported() asynchronous is a
difficult change?
(2) Let's discard the future when it doesn't transition in the timeout.
(3) We're going to silently say false if the future fails, can we log the
failure?
(4) 'if (' :)
(5) Can you add some newlines here to make it less dense?
- Ben Mahler
On Aug. 19, 2015, 12:57 a.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/
---
(Updated Aug. 19, 2015, 12:57 a.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
---
Perf supported() should be based on the version of perf, not the version of
the kernel.
Diffs
-
src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78
Diff: https://reviews.apache.org/r/37416/diff/
Testing
---
sudo make check
Thanks,
Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 13, 2015, 6:40 p.m.) Review request for mesos and Ben Mahler. Changes --- Address reviewer comments. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs (updated) - src/linux/perf.cpp cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/#review95231 --- Patch looks great! Reviews applied: [37423, 37424, 37417, 37416] All tests passed. - Mesos ReviewBot On Aug. 13, 2015, 12:53 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- (Updated Aug. 13, 2015, 12:53 a.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
Review Request 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37416/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- Perf supported() should be based on the version of perf, not the version of the kernel. Diffs - src/linux/perf.cpp cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 Diff: https://reviews.apache.org/r/37416/diff/ Testing --- sudo make check Thanks, Paul Brett
