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



src/linux/perf.hpp (line 71)
<https://reviews.apache.org/r/37540/#comment153551>

    we typically name the internal process with a process suffix. 
s/Listener/Process/



src/linux/perf.hpp (line 77)
<https://reviews.apache.org/r/37540/#comment153530>

    s/.././



src/linux/perf.hpp (line 78)
<https://reviews.apache.org/r/37540/#comment153531>

    put the argument on the next line.



src/linux/perf.hpp (lines 84 - 89)
<https://reviews.apache.org/r/37540/#comment153532>

    Do we need this overload considering users can call the overload below?



src/linux/perf.hpp (lines 88 - 89)
<https://reviews.apache.org/r/37540/#comment153535>

    swap the arguments.



src/linux/perf.hpp (lines 95 - 96)
<https://reviews.apache.org/r/37540/#comment153534>

    Swap the arguments.



src/linux/perf.hpp (line 111)
<https://reviews.apache.org/r/37540/#comment153536>

    argument on next line.



src/linux/perf.hpp (line 119)
<https://reviews.apache.org/r/37540/#comment153552>

    We use "_" prefix for continuation. Since this is just a private overload, 
I would remove "_".



src/linux/perf.cpp (lines 471 - 484)
<https://reviews.apache.org/r/37540/#comment153575>

    What is this for? Comments?



src/linux/perf.cpp (lines 496 - 497)
<https://reviews.apache.org/r/37540/#comment153577>

    kill this?



src/linux/perf.cpp (lines 501 - 503)
<https://reviews.apache.org/r/37540/#comment153576>

    kill this?



src/linux/perf.cpp (lines 514 - 516)
<https://reviews.apache.org/r/37540/#comment153580>

    swap the order.



src/linux/perf.cpp (line 522)
<https://reviews.apache.org/r/37540/#comment153579>

    why inline?
    
    should this be static?
    
    also, why is this defined here instead of outside the class as you did for 
other methods?



src/linux/perf.cpp (line 541)
<https://reviews.apache.org/r/37540/#comment153582>

    typically constructor and destructor are defined first.



src/linux/perf.cpp (lines 547 - 549)
<https://reviews.apache.org/r/37540/#comment153601>

    combine these.
    
    int ret = ...



src/linux/perf.cpp (lines 574 - 576)
<https://reviews.apache.org/r/37540/#comment153602>

    indent arguments.



src/linux/perf.cpp (line 577)
<https://reviews.apache.org/r/37540/#comment153603>

    new line.



src/linux/perf.cpp (lines 622 - 623)
<https://reviews.apache.org/r/37540/#comment153600>

    kill this. see below.



src/linux/perf.cpp (line 627)
<https://reviews.apache.org/r/37540/#comment153599>

    you can just use an initializer list here
    
    {cpu}



src/linux/perf.cpp (line 630)
<https://reviews.apache.org/r/37540/#comment153598>

    new line please.



src/linux/perf.cpp (line 720)
<https://reviews.apache.org/r/37540/#comment153597>

    s/pgsz/pageSize/



src/linux/perf.cpp (line 742)
<https://reviews.apache.org/r/37540/#comment153585>

    these should be defined in the following order
    
    ```
    read()
    
    _read()
    
    __read()
    
    ```



src/linux/perf.cpp (line 746)
<https://reviews.apache.org/r/37540/#comment153595>

    s/pgsz/pageSize/
    s/pgmsk/pageMask/ ?



src/linux/perf.cpp (line 790)
<https://reviews.apache.org/r/37540/#comment153593>

    why is __read() a separate function? can you just pull the logic down here?



src/linux/perf.cpp (line 817)
<https://reviews.apache.org/r/37540/#comment153587>

    put the .onReady() on the next line.



src/linux/perf.cpp (line 818)
<https://reviews.apache.org/r/37540/#comment153592>

    who can discard this future?



src/tests/containerizer/perf_tests.cpp (line 126)
<https://reviews.apache.org/r/37540/#comment153538>

    s/inherit/Inherit/



src/tests/containerizer/perf_tests.cpp (line 128)
<https://reviews.apache.org/r/37540/#comment153539>

    argument on next line.
    
    Can you add a comment on what this is trying to create? Not clear at all 
based on the arguments.



src/tests/containerizer/perf_tests.cpp (line 146)
<https://reviews.apache.org/r/37540/#comment153540>

    ditto.
    
    argument on next line.
    
    also, needs a comment on what this is creating.



src/tests/containerizer/perf_tests.cpp (line 151)
<https://reviews.apache.org/r/37540/#comment153542>

    ASSERT_SOME(event);



src/tests/containerizer/perf_tests.cpp (lines 156 - 157)
<https://reviews.apache.org/r/37540/#comment153548>

    s/context/Context/
    
    Can you add a better comment on what you are tryign to do here. "Causing 2 
context switches so that..?


- Vinod Kone


On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2015, 10:16 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2769
>     https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Abstract Linux kernel perf event API and provide API to collect schedule 
> events.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989 
>   src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea 
>   src/tests/containerizer/perf_tests.cpp 
> 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 
> 
> Diff: https://reviews.apache.org/r/37540/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Reply via email to