----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37462/#review97360 -----------------------------------------------------------
Looks good! Should be shippable after another round. src/linux/perf.hpp (line 56) <https://reviews.apache.org/r/37462/#comment153245> Whoops, doesn't this comment belong on the parse function? src/linux/perf.cpp (lines 278 - 281) <https://reviews.apache.org/r/37462/#comment153244> Hm.. can't we just add this as an overload of supported? ``` bool supported(); bool supported(const Version& version); ``` The first being only in the .cpp file, and let's stick it right below the existing supported to make the relationship clear. src/linux/perf.cpp (lines 324 - 353) <https://reviews.apache.org/r/37462/#comment153247> To keep it clean, how about we just do the supported check initially, and avoid the 'check' lambda entirely: ``` if (!supported()) { return Failure("Perf is not supported"); } internal::Perf* perf = new internal::Perf(argv); Future<string> output = perf->output(); spawn(perf, true); auto parse = [start, duration]( const Version& version, const string& output) -> Future<hashmap<string, mesos::PerfStatistics>> { Try<hashmap<string, mesos::PerfStatistics>> result = perf::parse(output, version); if (result.isError()) { return Failure("Failed to parse perf sample: " + result.error()); } foreachvalue (mesos::PerfStatistics& statistics, result.get()) { statistics.set_timestamp(start.secs()); statistics.set_duration(duration.secs()); } return result.get(); }; process::collect(perf::version(), output) .then(parse); ``` Note that there are races here either way, where the version we check might not match the version of perf that provided the sample. These lambdas are getting ugly :( src/linux/perf.cpp (line 391) <https://reviews.apache.org/r/37462/#comment153246> Let's just make an overload of supported that takes a version, see my comment above. src/linux/perf.cpp (line 408) <https://reviews.apache.org/r/37462/#comment153239> Let's just omit these shas since we can just look at the tags instead: https://github.com/torvalds/linux/blob/v4.0/tools/perf/perf.c Ditto below. src/linux/perf.cpp (lines 410 - 413) <https://reviews.apache.org/r/37462/#comment153238> How about just showing the two formats, and above where we call split() just saying that we use split because some fields may be empty (e.g. unit, see below). Seems a bit odd to show the empty unit cases here as extra cases. src/linux/perf.cpp (lines 414 - 417) <https://reviews.apache.org/r/37462/#comment153234> Braces please :) Also, this can just be a single statement? ``` if (tokens.size() == 4 || tokens.size() = 6) { ... } ``` src/linux/perf.cpp (lines 422 - 423) <https://reviews.apache.org/r/37462/#comment153240> Braces please :) src/linux/perf.cpp (lines 427 - 428) <https://reviews.apache.org/r/37462/#comment153241> Braces please :) src/linux/perf.cpp (line 430) <https://reviews.apache.org/r/37462/#comment153242> The caller will print the line, let's just say what's wrong here (i.e. unexpected number of tokens?). Also let's add a newline above this. src/linux/perf.cpp (line 443) <https://reviews.apache.org/r/37462/#comment153243> Do you have a tool that is removing spaces on if statements, or? src/tests/containerizer/perf_tests.cpp <https://reviews.apache.org/r/37462/#comment153248> Why did this get removed? src/tests/containerizer/perf_tests.cpp (line 69) <https://reviews.apache.org/r/37462/#comment153249> Please wrap these onto the next line, ditto below. - Ben Mahler On Sept. 1, 2015, 8:45 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37462/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2015, 8:45 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2834 > https://issues.apache.org/jira/browse/MESOS-2834 > > > Repository: mesos > > > Description > ------- > > Add support for version detection and parsing. > > > Diffs > ----- > > src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989 > src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee > src/tests/containerizer/perf_tests.cpp > 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8 > > Diff: https://reviews.apache.org/r/37462/diff/ > > > Testing > ------- > > sudo make check > > Perf tests may fail on many machines because the tests are currently using > the version of perf installed on the machine to choose decode. The next > patch will extend the perf tests to supply test cases for each of the > supported perf versions. > > > Thanks, > > Paul Brett > >
