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



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

    s/pathes/paths/



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

    one blank line.



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

    put the braces on the same line. more importantly, do you actually need a 
default constructor?
    
    also, do you want this class to be copyable or assignable? if not, make 
those constructors private too.



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

    i would move this function close to _create(), where it is used.



src/linux/perf.cpp (lines 564 - 565)
<https://reviews.apache.org/r/37540/#comment157926>

    please define this constructor outside the class as you did with the rest 
of the methods.



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

    1 blank line.



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

    1 blank line.



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

    no need for underscore.



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

    no need for underscore.



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

    I would define PerfEvent class methods after defining PerfEventProcess 
class methods.
    
    Also, please order the method definitions in the same order as their 
declarations.



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

    indent by two spaces.
    
    actually, you can even put this right above spawn if you like. that's what 
we do in most of our wrappers, afaict.



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

    Add a comment?



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

    instead of CHECK return an Error here?



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

    ditto. return Error.



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

    new line here.



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

    new line.



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

    The caller of this method doesn't know which cgroup failed, so how about 
including that info in the error?
    
    return Error("Failed to create PerfEvent for cgroup '" + cgroup + "': " + 
event.error());



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

    do you want an onDiscard() handler for this future? what should happen if 
the caller does a futuer.discard()?



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

    so the semantics of this read() are a bit confusing.
    
    if the caller calls the read() back to back, they will get the same event 
if the first io::poll() is still running? but they will get a different event 
if io::poll() has finished before the 2nd read() is called?



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

    s/reads,/reads./



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

    can we get the failure reason from io::poll?


- Vinod Kone


On Sept. 18, 2015, 10:11 p.m., Cong Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2015, 10:11 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 d10968ca670eb516bae08385fd0ddde8e8ad83b5 
>   src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a 
>   src/tests/containerizer/perf_tests.cpp 
> ed5212ee31b8aa47339b8b8fab184bbdf85be82a 
> 
> Diff: https://reviews.apache.org/r/37540/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Reply via email to