Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review94272 --- Mostly comments from the last review src/linux/perf.cpp (lines 295 - 296) https://reviews.apache.org/r/37045/#comment148811 This still isn't lined up? src/linux/perf.cpp (line 297) https://reviews.apache.org/r/37045/#comment148813 I think we're trying to avoid the blanket '=' if possible, looks like you only use 'this' here? Does this work if you capture 'this' only? src/linux/perf.cpp (line 301) https://reviews.apache.org/r/37045/#comment148817 Please add an explicit CHECK that this is not discarded, rather than relying on .get crashing src/linux/perf.cpp (line 303) https://reviews.apache.org/r/37045/#comment148816 Let's print the .failure of the future if it's failed. src/linux/perf.cpp (lines 308 - 309) https://reviews.apache.org/r/37045/#comment148819 No period for composition per the last comment, also WSTRINGIFY will print 'exited with status' already if appropriate, so this is double logging it. src/linux/perf.cpp (line 314) https://reviews.apache.org/r/37045/#comment148818 Please add an explicit CHECK that this is not discarded (otherwise calling .failure / .get crashing) - Ben Mahler On Aug. 5, 2015, 5:25 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- (Updated Aug. 5, 2015, 5:25 a.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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- (Updated Aug. 5, 2015, 6:23 p.m.) Review request for mesos and Ben Mahler. Changes --- Incorporate feedback from Ben Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Convert Linux perf sampler to use process:await(). Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37045/#review94278
---
Ship it!
Will get this committed now, thanks! I've made some comments so that you can
see what I've changed before committing.
src/linux/perf.cpp (lines 294 - 297)
https://reviews.apache.org/r/37045/#comment148831
This is where case 3 of wrapping is ok in the style guide.
src/linux/perf.cpp (line 300)
https://reviews.apache.org/r/37045/#comment148832
Looks like this isn't indented right? Seems to be even less indentation
than the body of `sample`.. also note from the style guide that the brace goes
on the line above.
src/linux/perf.cpp (lines 301 - 325)
https://reviews.apache.org/r/37045/#comment148834
I'd suggest wrapping these cases all together with an OptionError and
doing the promise failure / termination once if error.isSome, e.g.:
```
OptionError error;
if (...) {
error = ...;
} else if (...) {
error = ...;
} else if (...) {
error = ...;
}
if (error.isSome()) {
promise.fail(error.get().message);
terminate(self());
return;
}
```
src/linux/perf.cpp (line 322)
https://reviews.apache.org/r/37045/#comment148833
Whoops, don't you want the same discarded logic here? Otherwise, failure()
would crash.
- Ben Mahler
On Aug. 5, 2015, 6:23 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37045/
---
(Updated Aug. 5, 2015, 6:23 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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review94018 --- Patch looks great! Reviews applied: [37045] All tests passed. - Mesos ReviewBot 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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- (Updated Aug. 5, 2015, 5:25 a.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 (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review94156 --- Ship it! src/linux/perf.cpp (lines 292 - 294) https://reviews.apache.org/r/37045/#comment148689 The style checker doesn't catch this, but this isn't one of the styles for wrapping function calls in the style guide, can you wrap at the open paren? src/linux/perf.cpp (line 298) https://reviews.apache.org/r/37045/#comment148681 No need for this comment IMO src/linux/perf.cpp (line 301) https://reviews.apache.org/r/37045/#comment148690 Can you print the failure if the future is failed? Note that you can CHECK that it isn't discarded. src/linux/perf.cpp (lines 306 - 307) https://reviews.apache.org/r/37045/#comment148692 Note that we don't use periods when composing log messages, so: Failed to collect perf statistics: perf exited with status status Note that : is used for composition, so when we say which status we wouldn't use a : src/linux/perf.cpp (line 307) https://reviews.apache.org/r/37045/#comment148693 Can you use WSTRINGIFY instead here? Note that we should not be using WEXITSTATUS when WIFEXITED is not true. WSTRINGIFY will take care of this for you. src/linux/perf.cpp (line 312) https://reviews.apache.org/r/37045/#comment148694 Let's CHECK that this is not discarded as well, before we call .get src/linux/perf.cpp (line 314) https://reviews.apache.org/r/37045/#comment148680 Notice that we don't put a space before the : when composing failure messages, seems minor but inconsistent log message formatting is a huge pain. - Ben Mahler On Aug. 4, 2015, 11:59 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, 11:59 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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
On Aug. 4, 2015, 11:45 p.m., Ben Mahler wrote: src/linux/perf.cpp, lines 314-316 https://reviews.apache.org/r/37045/diff/2/?file=1028754#file1028754line314 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. I updated the error message because I realized that perf executed but returned an error status, which is of course different from failing to execute perf at all (caught be the test on line 301). - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review94148 --- 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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- 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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review94152 --- Patch looks great! Reviews applied: [37045] All tests passed. - Mesos ReviewBot 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
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- (Updated Aug. 4, 2015, 11:59 p.m.) Review request for mesos and Ben Mahler. Changes --- Incorporate review comments. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Convert Linux perf sampler to use process:await(). Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- 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. Changes --- Address review comments. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Convert Linux perf sampler to use process:await(). Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- 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 Futurestring 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 FutureOptionint, Futurestring, Futurestring results) - FutureNothing ``` 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 FutureNothing 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? ``` FutureOptionint status = std::get0(results); Futurestring output = std::get1(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
Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- 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 (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing (updated) --- sudo make check Thanks, Paul Brett
