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 Wed, 05 Aug 2015 00:55:45 GMT

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


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