mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 37045: Convert Linux perf sampler to use process:await().
Date Mon, 03 Aug 2015 21:28:41 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message