mesos-reviews mailing list archives

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



src/linux/perf.cpp (line 449)
<https://reviews.apache.org/r/36378/#comment144639>

    Why a Try<> argument and then a CHECK()? The caller of os::release() should check
the return so either check it in supported() or query and check it in _supported().
    
    Also, should be const Version&.



src/linux/perf.cpp (lines 468 - 473)
<https://reviews.apache.org/r/36378/#comment144642>

    We should document this behavior at os::release() and move this there as an alternative.
    
    If it's kept in here then make static?
    
    Argument should be named 'version', not 'v'.
    
    Does it need to have such a long function name, particularly if it's local?
    
    Version canonicalize(const Version& version).



src/linux/perf.cpp (line 481)
<https://reviews.apache.org/r/36378/#comment144544>

    Suggest moving the TODO to here.



src/linux/perf.cpp (lines 484 - 485)
<https://reviews.apache.org/r/36378/#comment144545>

    There's no map, it returns a single Sample?



src/linux/perf.cpp (lines 486 - 488)
<https://reviews.apache.org/r/36378/#comment144645>

    This fits on a single line.



src/linux/perf.cpp (lines 490 - 492)
<https://reviews.apache.org/r/36378/#comment144546>

    newlines, please ;-)



src/linux/perf.cpp (line 492)
<https://reviews.apache.org/r/36378/#comment144648>

    Can you provide links to the documentation/commits that introduced these changes?



src/linux/perf.cpp (line 508)
<https://reviews.apache.org/r/36378/#comment144549>

    Check how patch versions are handled is correct, e.g., this would match 3.12.1 (if it
existed) when I think the change was in 3.13.0 so it should be >= 3.13.0



src/linux/perf.cpp (line 524)
<https://reviews.apache.org/r/36378/#comment144649>

    newline



src/linux/perf.cpp (line 532)
<https://reviews.apache.org/r/36378/#comment144651>

    const T&



src/linux/perf.cpp (line 536)
<https://reviews.apache.org/r/36378/#comment144550>

    newline


- Ian Downes


On July 9, 2015, 4:08 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36378/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 4:08 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-2834
>     https://issues.apache.org/jira/browse/MESOS-2834
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending
on kernel version.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 
> 
> Diff: https://reviews.apache.org/r/36378/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>