----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37423/#review95456 -----------------------------------------------------------
Ship it! Thanks Paul, just some cleanups and we can get this committed. src/linux/perf.cpp (lines 158 - 161) <https://reviews.apache.org/r/37423/#comment150657> s/Execute/Executes/ s/parameters/arguments/ Feel free to add a TODO for me: ``` TODO(bmahler): Add a process::os::shell to generalize this. ``` src/linux/perf.cpp (line 159) <https://reviews.apache.org/r/37423/#comment150658> s/returning/returns/ src/linux/perf.cpp (lines 185 - 190) <https://reviews.apache.org/r/37423/#comment150655> Can you remove duration entirely from `Perf` now that it's not used? src/linux/perf.cpp (lines 318 - 319) <https://reviews.apache.org/r/37423/#comment150654> Can you update this message since we're not necessarily collecting perf statistics now? src/linux/perf.cpp (lines 353 - 354) <https://reviews.apache.org/r/37423/#comment150403> Can you remove this comment? 'PerfSampler' seems to refer to an older version of this patch. src/linux/perf.cpp (lines 359 - 363) <https://reviews.apache.org/r/37423/#comment150642> Could you add some newlines for readability? ``` Time start = Clock::now(); Perf* sampler = new Perf(argv, duration); Future<string> future = sampler->future(); spawn(sampler, true); return future.then([=] (const Future<string>& output) -> ``` src/linux/perf.cpp (line 363) <https://reviews.apache.org/r/37423/#comment150643> Can you capture 'start' and 'duration' explicitly rather than all values? src/linux/perf.cpp (line 365) <https://reviews.apache.org/r/37423/#comment150645> If you look at the style guide, this should be up on the previous line FWICT src/linux/perf.cpp (lines 366 - 381) <https://reviews.apache.org/r/37423/#comment150646> This is indented too much, can you indent based on the style guide for lambdas? src/linux/perf.cpp (line 367) <https://reviews.apache.org/r/37423/#comment150647> = wrapping should be indented by 2 (this uses 4) src/linux/perf.cpp (lines 369 - 371) <https://reviews.apache.org/r/37423/#comment150648> How about tacking on the failure reason: ``` "Failed to parse perf sample: " + ... ``` src/linux/perf.cpp (lines 373 - 380) <https://reviews.apache.org/r/37423/#comment150652> No need for the non-const copy, you can just use .get() to alter the value directly. In the future, you could use `->` which will be a lot cleaner syntactically: https://issues.apache.org/jira/browse/MESOS-2757 src/linux/perf.cpp (lines 406 - 407) <https://reviews.apache.org/r/37423/#comment150641> This should fit on one line? src/linux/perf.cpp (lines 433 - 434) <https://reviews.apache.org/r/37423/#comment150638> Having to pass the duration into argv and sample seems a bit messy, but I don't have a suggestion for an easy way to clean it up. Should fit on one line? ``` >>> len(' return internal::sample(internal::argv(events, cgroups, duration), duration);') 79 ``` - Ben Mahler On Aug. 13, 2015, 12:19 a.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37423/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2015, 12:19 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3185 > https://issues.apache.org/jira/browse/MESOS-3185 > > > Repository: mesos > > > Description > ------- > > Split out common functions for running "perf" into a common perf class with > wrapper functions to allow for reuse between sample, valid and version > operations. > > > Diffs > ----- > > src/linux/perf.cpp cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 > > Diff: https://reviews.apache.org/r/37423/diff/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Paul Brett > >
