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



src/linux/perf.cpp (lines 384 - 385)
<https://reviews.apache.org/r/37416/#comment150952>

    Any reason to put this in internal::? It seems nice to have this as 
perf::version, even though it's only in the cpp file.



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

    const string&
    
    how about a newline for readability?



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

    End comments with a period please :)



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

    Why `_supported` here that takes a version? Why not just have supported 
compute the version and then perform the necessary check?



src/linux/perf.cpp (lines 480 - 481)
<https://reviews.apache.org/r/37416/#comment150954>

    Perf versions are numbered similarly to linux versions..?
    
    Not yours but mind ending it with a period?



src/linux/perf.cpp (lines 483 - 489)
<https://reviews.apache.org/r/37416/#comment150955>

    Couple of things:
    
    (1) Let's add a comment as to why we're using await here, since it is an 
anti-pattern. Namely, is it because making supported() asynchronous is a 
difficult change?
    
    (2) Let's discard the future when it doesn't transition in the timeout.
    
    (3) We're going to silently say false if the future fails, can we log the 
failure?
    
    (4) 'if (' :)
    
    (5) Can you add some newlines here to make it less dense?


- Ben Mahler


On Aug. 19, 2015, 12:57 a.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37416/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2015, 12:57 a.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
> 
>

Reply via email to