You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/08/03 23:10:32 UTC

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

Re: Review Request 37045: Convert Linux perf sampler to use process:await().

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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().

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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().

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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 Option<Error> and doing the promise failure / termination once if error.isSome, e.g.:
    
    ```
    Option<Error> 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().

Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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().

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37045/#review94235
-----------------------------------------------------------


Patch looks great!

Reviews applied: [37045]

All tests passed.

- Mesos ReviewBot


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().

Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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().

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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().

Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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().

Posted by Paul Brett <pa...@twopensource.com>.

> 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().

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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().

Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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().

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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().

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
> 
>