----------------------------------------------------------- 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 > >
