----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37423/#review95771 -----------------------------------------------------------
Ship it! Will get this committed now, thanks Paul! src/linux/perf.cpp (line 167) <https://reviews.apache.org/r/37423/#comment150885> This seems a little brittle, because the caller may have already put 'perf' as the first argument, no? I'll add a note as to why we need this hack. src/linux/perf.cpp (line 183) <https://reviews.apache.org/r/37423/#comment150886> newline for readability? src/linux/perf.cpp (line 344) <https://reviews.apache.org/r/37423/#comment150887> PerfSampler doesn't exist..? We can just remove this comment altogether IMO. src/linux/perf.cpp (lines 356 - 370) <https://reviews.apache.org/r/37423/#comment150888> Recall that with .then your lambda doesn't need to take a Future since it only composes when the future succeeds, normally calling .get() on a Future would need to be guarded to ensure the future is ready. Ideally we could compose with Try here: ``` auto stamp = [start, duration] ( const hashmap<string, mesos::PerfStatistics>& statistics) { foreachvalue (mesos::PerfStatistics& s, statistics) { s.set_timestamp(start.secs()); s.set_duration(duration.secs()); } return stats; } return future .then(perf::parse) .then(stamp); ``` But we can't compose like this currently, so I'll just pull out a single lambda for parsing / stamping. - Ben Mahler On Aug. 18, 2015, 6:10 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37423/ > ----------------------------------------------------------- > > (Updated Aug. 18, 2015, 6:10 p.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 > >
