This is an automatically generated e-mail. To reply, visit:

src/linux/perf.hpp (line 96)


src/linux/perf.hpp (line 107)

    one blank line.

src/linux/perf.hpp (line 110)

    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)

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

src/linux/perf.cpp (lines 564 - 565)

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

src/linux/perf.cpp (line 569)

    1 blank line.

src/linux/perf.cpp (line 573)

    1 blank line.

src/linux/perf.cpp (line 579)

    no need for underscore.

src/linux/perf.cpp (line 581)

    no need for underscore.

src/linux/perf.cpp (line 590)

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

src/linux/perf.cpp (line 591)

    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)

    Add a comment?

src/linux/perf.cpp (line 633)

    instead of CHECK return an Error here?

src/linux/perf.cpp (line 667)

    ditto. return Error.

src/linux/perf.cpp (line 679)

    new line here.

src/linux/perf.cpp (line 717)

    new line.

src/linux/perf.cpp (line 750)

    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 + "': " + 

src/linux/perf.cpp (line 782)

    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)

    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)


src/linux/perf.cpp (line 871)

    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