----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review92128 -----------------------------------------------------------
Thanks Paul for taking this on! See my detailed comments. We should first pulling the subprocess logic into 'perf::execute' first. Then, we can add version support in the subsequent patch. I haven't looked at the version stuff yet. src/linux/perf.cpp (line 162) <https://reviews.apache.org/r/36378/#comment146076> Kill this line. src/linux/perf.cpp (line 163) <https://reviews.apache.org/r/36378/#comment146071> s/commandResults/Output. src/linux/perf.cpp (line 165) <https://reviews.apache.org/r/36378/#comment146072> s/exitCode/status/ src/linux/perf.cpp (line 166) <https://reviews.apache.org/r/36378/#comment146073> s/stdOut/out/ src/linux/perf.cpp (lines 166 - 167) <https://reviews.apache.org/r/36378/#comment146075> Do you need std:: prefix in a cpp file? Can you just use 'using std::string' in the begining? Please do a sweep to cleanup all the occurances in this file. src/linux/perf.cpp (line 167) <https://reviews.apache.org/r/36378/#comment146074> s/stdErr/err/ src/linux/perf.cpp (lines 171 - 173) <https://reviews.apache.org/r/36378/#comment146070> Pulling this out is great! But can we have a patch first to do this refactor first, and then add the 'version' stuff in the second patch? src/linux/perf.cpp (lines 171 - 173) <https://reviews.apache.org/r/36378/#comment146077> I would s/executeCommand/execute/ since you already have a 'cmd' in the argument list. I would rather make this function specific to perf. For example: (no need the 'cmd') When MESOS-3035 gets in, you can just modify perf::execute implementation to use that instead. ``` perf::execute({"--version"}); perf::execute({"stat", "--all-cpus"}); .... ``` src/linux/perf.cpp (line 178) <https://reviews.apache.org/r/36378/#comment146080> s/execCmd/s/ src/linux/perf.cpp (lines 179 - 183) <https://reviews.apache.org/r/36378/#comment146079> Please fix the indentation. src/linux/perf.cpp (line 181) <https://reviews.apache.org/r/36378/#comment146078> Do you need 'stdin'? Should that be Subprocess::PATH("/dev/null")? src/linux/perf.cpp (line 189) <https://reviews.apache.org/r/36378/#comment146081> No need for this temp variable. src/linux/perf.cpp (lines 191 - 207) <https://reviews.apache.org/r/36378/#comment146084> As Ben suggested in the email thread, you can do: ``` process::await( perf.get().status(), io::read(perf.get().out().get()), io::read(perf.get().err().get())) .then([](...) -> Future<Output> { ... }); ``` You don't need the lambda capture anymore if you wrote code like above. Please take a look at https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/posix/disk.cpp#L428 src/linux/perf.cpp (line 203) <https://reviews.apache.org/r/36378/#comment146087> I would rather let the caller dealing with extracting exit code. So returning 'status.get()' is better. src/linux/perf.cpp (lines 358 - 383) <https://reviews.apache.org/r/36378/#comment146089> Do you want to simplify this as well? I think 'perf::execute' should use 'setupChild' as well. See the following for details: https://issues.apache.org/jira/browse/MESOS-2462 https://reviews.apache.org/r/32805/ - Jie Yu On July 17, 2015, 1:30 a.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36378/ > ----------------------------------------------------------- > > (Updated July 17, 2015, 1:30 a.m.) > > > Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Jie Yu, and Cong > Wang. > > > Bugs: MESOS-2834 > https://issues.apache.org/jira/browse/MESOS-2834 > > > Repository: mesos > > > Description > ------- > > Refactor Linux Performance monitor to handle changing 'perf stat' output > versions depending on kernel version. > > > Diffs > ----- > > src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 > > Diff: https://reviews.apache.org/r/36378/diff/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Paul Brett > >
