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 37416: Perf supported() should be based on the version of perf, not the version of the kernel.
Date Mon, 31 Aug 2015 21:45:56 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37416/#review97153
-----------------------------------------------------------



src/linux/perf.cpp (lines 392 - 397)
<https://reviews.apache.org/r/37416/#comment152917>

    (1) How about a newline above the return for readability? Also it's what we did in sample().
    
    (2) .then should be indented by 2 spaces.
    
    (3) Why are you capturing everything by value? Can you use an empty catpure list here?



src/linux/perf.cpp (lines 417 - 418)
<https://reviews.apache.org/r/37416/#comment152918>

    Why did you remove the braces here?



src/linux/perf.cpp (line 418)
<https://reviews.apache.org/r/37416/#comment152919>

    Why did you add a period here?



src/linux/perf.cpp (lines 442 - 443)
<https://reviews.apache.org/r/37416/#comment152920>

    Ditto here.



src/linux/perf.cpp (lines 464 - 468)
<https://reviews.apache.org/r/37416/#comment152921>

    `_parse` is not using this yet, so can we remove this comment and just inline `_supported`
inside `supported` in this patch?



src/linux/perf.cpp (lines 470 - 471)
<https://reviews.apache.org/r/37416/#comment152924>

    Not yours, but this comment doesn't reflect the code..?



src/linux/perf.cpp (lines 473 - 474)
<https://reviews.apache.org/r/37416/#comment152926>

    Can you avoid the "jagged" look of this comment?



src/linux/perf.cpp (line 476)
<https://reviews.apache.org/r/37416/#comment152927>

    newline here?



src/linux/perf.cpp (lines 477 - 478)
<https://reviews.apache.org/r/37416/#comment152929>

    Can we clarify this logging message? It's a bit unclear what went wrong. Specifically
if it timed out, let's say so. If it failed, let's print the failure.
    
    Also, please do a discard in this case so that we don't leave around a growing number
of hanging perf processes.



src/linux/perf.cpp (line 487)
<https://reviews.apache.org/r/37416/#comment152930>

    Per the comment above, let's just inline this for now, we can look at `_supported` in
one of the subsequent patches (which is where I assume you wanted this to be split).


- Ben Mahler


On Aug. 31, 2015, 7:41 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37416/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2015, 7:41 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Perf supported() should be based on the version of perf, not the version of the kernel.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
> 
> Diff: https://reviews.apache.org/r/37416/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>


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