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