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/08/13 02:53:55 UTC
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
Re: Review Request 37416: Perf supported() should be based on the
version of perf, not the version of the kernel.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37416: Perf supported() should be based on the
version of perf, not the version of the kernel.
Posted by Paul Brett <pa...@twopensource.com>.
> 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.
Posted by Cong Wang <cw...@twopensource.com>.
> 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.
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review97153
-----------------------------------------------------------
src/linux/perf.cpp (lines 392 - 397)
<https://reviews.apache.org/r/37416/#comment152917>
(1) How about a newline above the return for readability? Also it's what we did in sample().
(2) .then should be indented by 2 spaces.
(3) Why are you capturing everything by value? Can you use an empty catpure list here?
src/linux/perf.cpp (lines 417 - 418)
<https://reviews.apache.org/r/37416/#comment152918>
Why did you remove the braces here?
src/linux/perf.cpp (line 418)
<https://reviews.apache.org/r/37416/#comment152919>
Why did you add a period here?
src/linux/perf.cpp (lines 442 - 443)
<https://reviews.apache.org/r/37416/#comment152920>
Ditto here.
src/linux/perf.cpp (lines 464 - 468)
<https://reviews.apache.org/r/37416/#comment152921>
`_parse` is not using this yet, so can we remove this comment and just inline `_supported` inside `supported` in this patch?
src/linux/perf.cpp (lines 470 - 471)
<https://reviews.apache.org/r/37416/#comment152924>
Not yours, but this comment doesn't reflect the code..?
src/linux/perf.cpp (lines 473 - 474)
<https://reviews.apache.org/r/37416/#comment152926>
Can you avoid the "jagged" look of this comment?
src/linux/perf.cpp (line 476)
<https://reviews.apache.org/r/37416/#comment152927>
newline here?
src/linux/perf.cpp (lines 477 - 478)
<https://reviews.apache.org/r/37416/#comment152929>
Can we clarify this logging message? It's a bit unclear what went wrong. Specifically if it timed out, let's say so. If it failed, let's print the failure.
Also, please do a discard in this case so that we don't leave around a growing number of hanging perf processes.
src/linux/perf.cpp (line 487)
<https://reviews.apache.org/r/37416/#comment152930>
Per the comment above, let's just inline this for now, we can look at `_supported` in one of the subsequent patches (which is where I assume you wanted this to be split).
- Ben Mahler
On Aug. 31, 2015, 7:41 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> 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
> -----
>
> 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.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review97172
-----------------------------------------------------------
Ship it!
Some stuff leftover from the last review comments, I'll make the updates and commit for you shortly.
src/linux/perf.cpp (line 393)
<https://reviews.apache.org/r/37416/#comment152961>
two space indent for .then
src/linux/perf.cpp (line 474)
<https://reviews.apache.org/r/37416/#comment152962>
if (
src/linux/perf.cpp (lines 474 - 482)
<https://reviews.apache.org/r/37416/#comment152965>
Hm.. you didn't add the explicit discard here?
```
if (!version.isReady()) {
if (version.isFailed()) {
LOG(ERROR) << "Failed to get perf version: " << version.failure();
} else {
LOG(ERROR) << "Failed to get perf version: timeout of 5secs exceeded";
}
version.discard();
return false;
}
```
src/linux/perf.cpp (line 478)
<https://reviews.apache.org/r/37416/#comment152964>
This case also needs to return!
src/linux/perf.cpp (line 479)
<https://reviews.apache.org/r/37416/#comment152963>
extra space here?
- Ben Mahler
On Aug. 31, 2015, 10:27 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> 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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
> 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.
Posted by Paul Brett <pa...@twopensource.com>.
> 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.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review95235
-----------------------------------------------------------
src/linux/perf.cpp (line 411)
<https://reviews.apache.org/r/37416/#comment150084>
s/PerfVersion/Perf::version/
src/linux/perf.cpp (line 510)
<https://reviews.apache.org/r/37416/#comment150080>
These comments need to update too?
src/linux/perf.cpp (line 513)
<https://reviews.apache.org/r/37416/#comment150081>
reasonable
src/linux/perf.cpp (line 514)
<https://reviews.apache.org/r/37416/#comment150082>
unavailable
src/linux/perf.cpp (line 516)
<https://reviews.apache.org/r/37416/#comment150083>
!isReady() contains isFailed(), right?
- Cong Wang
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
>
>