----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37462/#review97516 -----------------------------------------------------------
Ship it! I've made adjustments based on the feedback below before committing, please take a look over the comments to make sure they all make sense to you. src/linux/perf.cpp (line 271) <https://reviews.apache.org/r/37462/#comment153453> Missed this earlier, s/=// src/linux/perf.cpp (line 287) <https://reviews.apache.org/r/37462/#comment153449> Looks like this comment belongs in the overload now? src/linux/perf.cpp (lines 348 - 351) <https://reviews.apache.org/r/37462/#comment153458> If you use process::collect instead of process::await, this gets cleaned up substantially, you can deal directly with the values rather than the transitioned Futures. src/linux/perf.cpp (line 352) <https://reviews.apache.org/r/37462/#comment153462> I'd suggest we just unpack the tuple at the top, otherwise it looks pretty verbose. Note that this would be similar to adding an `unpacked` function wrapper as michael suggested here: https://issues.apache.org/jira/browse/MESOS-3188 src/linux/perf.cpp (lines 358 - 359) <https://reviews.apache.org/r/37462/#comment153459> The reason should come after the colon, so this reads a bit strangely: Perf version unsupported: 2.6.38 How about: Perf 2.6.38 is not supported src/linux/perf.cpp (lines 383 - 384) <https://reviews.apache.org/r/37462/#comment153521> Much like we don't wait for version and output forever in perf::supported, let's add a TODO here to not wait forever: ``` // TODO(pbrett): Don't wait for these forever. ``` src/linux/perf.cpp (line 384) <https://reviews.apache.org/r/37462/#comment153454> Generally we try to put the .then on the next line, as if it was a statement: ``` return process::collect(perf::version(), output) .then(parse); ``` src/linux/perf.cpp <https://reviews.apache.org/r/37462/#comment153447> Seems minor, but makes life easier for reviewers when code movement is pulled out in separate patches as the diff is easier to read. :) src/linux/perf.cpp (line 417) <https://reviews.apache.org/r/37462/#comment153442> We already say this in the comment for this function, and it's shown in the call to split, do we need to say it again? src/linux/perf.cpp (lines 419 - 421) <https://reviews.apache.org/r/37462/#comment153444> How about putting the format on a separate line as you did before? ``` // Optional running time and ratio were introduced in Linux v4.0, // which make the format either: // value,unit,event,cgroup // value,unit,event,cgroup,running,ratio ``` src/linux/perf.cpp (lines 468 - 469) <https://reviews.apache.org/r/37462/#comment153440> It's a little bit easier to avoid missing close quotes when it's on the same line: ``` return Error("Unexpected event '" + sample->event + "'" " in perf output at line: " + line); ``` - Ben Mahler On Sept. 2, 2015, 6:52 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37462/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2015, 6:52 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2834 > https://issues.apache.org/jira/browse/MESOS-2834 > > > Repository: mesos > > > Description > ------- > > Add support for version detection and parsing. > > > Diffs > ----- > > src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989 > src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea > src/tests/containerizer/perf_tests.cpp > 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 > > Diff: https://reviews.apache.org/r/37462/diff/ > > > Testing > ------- > > sudo make check > > Perf tests may fail on many machines because the tests are currently using > the version of perf installed on the machine to choose decode. The next > patch will extend the perf tests to supply test cases for each of the > supported perf versions. > > > Thanks, > > Paul Brett > >
