----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review94148 -----------------------------------------------------------
Looks great paul! Just some minor things around style before we can get this committed. src/linux/perf.cpp (lines 295 - 296) <https://reviews.apache.org/r/37045/#comment148668> This isn't a 'future' so this name seems a little counter-intuitive, how about we call this something like 'results'? src/linux/perf.cpp (line 301) <https://reviews.apache.org/r/37045/#comment148672> This is indented strangely, did you look over the diff? Also, notice that we don't use periods at the end of any of our failure messages or log statements, this is because it is difficult to end with one period consistently when composing error messages. src/linux/perf.cpp (lines 306 - 307) <https://reviews.apache.org/r/37045/#comment148669> Any reason you're changing the style of the failure messages? Let's leave them untouched in this patch, since they look goot to me. src/linux/perf.cpp (line 313) <https://reviews.apache.org/r/37045/#comment148671> I guess the sytle checker is not catching this, but if you look around the rest of the code, we add a space between if and the open paren. Can you do a sweep? src/linux/perf.cpp (line 314) <https://reviews.apache.org/r/37045/#comment148670> Ditto here, can can we print the same style message as before? ``` "Failed to read output of perf process: " + ... ``` Note the format of these messages "Failed to ...: <reason>" - Ben Mahler On Aug. 4, 2015, 4:57 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37045/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2015, 4:57 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2834 > https://issues.apache.org/jira/browse/MESOS-2834 > > > Repository: mesos > > > Description > ------- > > Convert Linux perf sampler to use process:await(). > > > Diffs > ----- > > src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 > > Diff: https://reviews.apache.org/r/37045/diff/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Paul Brett > >
