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 Tue, 04 Aug 2015 23:45:01 GMT

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


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