----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review93968 -----------------------------------------------------------
Looks pretty good, thanks Paul! Just a couple bits of feedback around failure handling and use of .then. src/linux/perf.cpp (line 176) <https://reviews.apache.org/r/37045/#comment148432> Why the change here? Mind reverting this? src/linux/perf.cpp (lines 292 - 294) <https://reviews.apache.org/r/37045/#comment148435> Hm.. why the explicit Future<string> wrapping here? Also, mind lining things up on the open paren here? src/linux/perf.cpp (lines 295 - 297) <https://reviews.apache.org/r/37045/#comment148440> Couple of things here: (1) .then performs composition, so only if the future succeeds (ready) will the callback be invoked, this means you don't need the outer future type in the callback: ``` .then([=](const std::tuple< Future<Option<int>>, Future<string>, Future<string>>& results) -> Future<Nothing> ``` But since we don't care about composition here, we should just use .onReady, which will avoid the need to return Failures to satisfy the Future<Nothing> return type, we can just have a void continuation. (2) Any reason to switch to a lambda for this? Note that you need to at least 'defer' to this lambda, as before. Otherwise, your continuation will execute without locking the actor! src/linux/perf.cpp (lines 299 - 305) <https://reviews.apache.org/r/37045/#comment148441> Per the above comments, this will never happen since .then is passing a tuple directly, not a Future. src/linux/perf.cpp (lines 306 - 310) <https://reviews.apache.org/r/37045/#comment148442> We can't call .get() on each individual future until we've validated that it is ready. How about pulling out the things we're interested in? ``` Future<Option<int>> status = std::get<0>(results); Future<string> output = std::get<1>(results); ``` Then validating that these are not failed. - Ben Mahler On Aug. 3, 2015, 9:10 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37045/ > ----------------------------------------------------------- > > (Updated Aug. 3, 2015, 9:10 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 > >
