----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review91253 -----------------------------------------------------------
src/linux/perf.cpp (line 449) <https://reviews.apache.org/r/36378/#comment144639> Why a Try<> argument and then a CHECK()? The caller of os::release() should check the return so either check it in supported() or query and check it in _supported(). Also, should be const Version&. src/linux/perf.cpp (lines 468 - 473) <https://reviews.apache.org/r/36378/#comment144642> We should document this behavior at os::release() and move this there as an alternative. If it's kept in here then make static? Argument should be named 'version', not 'v'. Does it need to have such a long function name, particularly if it's local? Version canonicalize(const Version& version). src/linux/perf.cpp (line 481) <https://reviews.apache.org/r/36378/#comment144544> Suggest moving the TODO to here. src/linux/perf.cpp (lines 484 - 485) <https://reviews.apache.org/r/36378/#comment144545> There's no map, it returns a single Sample? src/linux/perf.cpp (lines 486 - 488) <https://reviews.apache.org/r/36378/#comment144645> This fits on a single line. src/linux/perf.cpp (lines 490 - 492) <https://reviews.apache.org/r/36378/#comment144546> newlines, please ;-) src/linux/perf.cpp (line 492) <https://reviews.apache.org/r/36378/#comment144648> Can you provide links to the documentation/commits that introduced these changes? src/linux/perf.cpp (line 508) <https://reviews.apache.org/r/36378/#comment144549> Check how patch versions are handled is correct, e.g., this would match 3.12.1 (if it existed) when I think the change was in 3.13.0 so it should be >= 3.13.0 src/linux/perf.cpp (line 524) <https://reviews.apache.org/r/36378/#comment144649> newline src/linux/perf.cpp (line 532) <https://reviews.apache.org/r/36378/#comment144651> const T& src/linux/perf.cpp (line 536) <https://reviews.apache.org/r/36378/#comment144550> newline - Ian Downes On July 9, 2015, 4:08 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36378/ > ----------------------------------------------------------- > > (Updated July 9, 2015, 4:08 p.m.) > > > Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. > > > Bugs: MESOS-2834 > https://issues.apache.org/jira/browse/MESOS-2834 > > > Repository: mesos > > > Description > ------- > > Refactor Linux Performance monitor to handle changing 'perf stat' output > versions depending on kernel version. > > > Diffs > ----- > > src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 > > Diff: https://reviews.apache.org/r/36378/diff/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Paul Brett > >
