----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21443/#review44409 -----------------------------------------------------------
Hey Ian, took a look at the API being exposed and a quick look through the implementation. src/linux/perf.hpp <https://reviews.apache.org/r/21443/#comment78783> What is being returned? I can't intuit what the keys would be in this map without looking at the implementation. Can you add comments here and then if we don't need to return maps then can we simplify this and just directly return the PerfStatistics? src/linux/perf.hpp <https://reviews.apache.org/r/21443/#comment78785> What about: // Returns whether the events are valid. bool valid(const set<string>& events); This then seems to read a bit better to callers: if (perf::valid(events)) { } vs. if (perf::validate(events)) { } Because we're asking a question about the events it seems a bit easier to read it as 'valid' or 'isValid'. 'validate' sounds like an action more so than a question. src/linux/perf.hpp <https://reviews.apache.org/r/21443/#comment78786> Should we put this in an 'internal' namespace to at least make it harder to get at for callers? Let's document what it returns, that is, what are the keys? src/linux/perf.cpp <https://reviews.apache.org/r/21443/#comment78788> Maybe a comment as to why the _nested_ loop here is needed? src/linux/perf.cpp <https://reviews.apache.org/r/21443/#comment78789> Why not just group the comments and statements: // Start reading from stdout and stderr now. We don't use // stderr but must read from it to avoid blocking the child. output.push_back(process::io::read(perf.get().out())); output.push_back(process::io::read(perf.get().err())); src/linux/perf.cpp <https://reviews.apache.org/r/21443/#comment78790> The future failure message should be enough here, since this is designed as a library we should avoid logging internally in a redundant manner with what we send back to the caller. src/linux/perf.cpp <https://reviews.apache.org/r/21443/#comment78787> - Ben Mahler On May 29, 2014, 10:07 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21443/ > ----------------------------------------------------------- > > (Updated May 29, 2014, 10:07 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1278 > https://issues.apache.org/jira/browse/MESOS-1278 > > > Repository: mesos-git > > > Description > ------- > > Perf can be run against either a set of pids or a perf_event cgroup. > > I decided to parse the perf output into the protobuf directly, rather than > via JSON, because it was simpler to handle output like <not supported> and > <not counted>. Plus JSON's floating point numbers cannot fully represent > uint64. > > > Diffs > ----- > > src/Makefile.am ffde59be8683dd40cc5bc7cb88cd88c5bc91cf96 > src/linux/perf.hpp PRE-CREATION > src/linux/perf.cpp PRE-CREATION > src/tests/cgroups_tests.cpp 5f674cd678e67f10bfef4620d927bb5af7c93753 > src/tests/perf_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/21443/diff/ > > > Testing > ------- > > Added tests for pids and cgroups. > > > Thanks, > > Ian Downes > >
