You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Paul Brett <pa...@twopensource.com> on 2015/09/01 21:47:49 UTC
Re: Review Request 37462: Add support for version detection and
parsing.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37462/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 7:47 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Updated to reflect removal of PID sampling.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Add support for version detection and parsing.
Diffs (updated)
-----
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
Re: Review Request 37462: Add support for version detection and
parsing.
Posted by Paul Brett <pa...@twopensource.com>.
> On Sept. 1, 2015, 11:43 p.m., Ben Mahler wrote:
> > src/linux/perf.hpp, line 56
> > <https://reviews.apache.org/r/37462/diff/7/?file=1061618#file1061618line56>
> >
> > Whoops, doesn't this comment belong on the parse function?
Actually, it applies to both of them. I'll make that clear.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37462/#review97360
-----------------------------------------------------------
On Sept. 2, 2015, 6:52 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37462/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 6:52 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 bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> 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
>
>
Re: Review Request 37462: Add support for version detection and
parsing.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37462: Add support for version detection and
parsing.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37462/#review97516
-----------------------------------------------------------
Ship it!
I've made adjustments based on the feedback below before committing, please take a look over the comments to make sure they all make sense to you.
src/linux/perf.cpp (line 271)
<https://reviews.apache.org/r/37462/#comment153453>
Missed this earlier, s/=//
src/linux/perf.cpp (line 287)
<https://reviews.apache.org/r/37462/#comment153449>
Looks like this comment belongs in the overload now?
src/linux/perf.cpp (lines 348 - 351)
<https://reviews.apache.org/r/37462/#comment153458>
If you use process::collect instead of process::await, this gets cleaned up substantially, you can deal directly with the values rather than the transitioned Futures.
src/linux/perf.cpp (line 352)
<https://reviews.apache.org/r/37462/#comment153462>
I'd suggest we just unpack the tuple at the top, otherwise it looks pretty verbose. Note that this would be similar to adding an `unpacked` function wrapper as michael suggested here:
https://issues.apache.org/jira/browse/MESOS-3188
src/linux/perf.cpp (lines 358 - 359)
<https://reviews.apache.org/r/37462/#comment153459>
The reason should come after the colon, so this reads a bit strangely:
Perf version unsupported: 2.6.38
How about:
Perf 2.6.38 is not supported
src/linux/perf.cpp (lines 383 - 384)
<https://reviews.apache.org/r/37462/#comment153521>
Much like we don't wait for version and output forever in perf::supported, let's add a TODO here to not wait forever:
```
// TODO(pbrett): Don't wait for these forever.
```
src/linux/perf.cpp (line 384)
<https://reviews.apache.org/r/37462/#comment153454>
Generally we try to put the .then on the next line, as if it was a statement:
```
return process::collect(perf::version(), output)
.then(parse);
```
src/linux/perf.cpp
<https://reviews.apache.org/r/37462/#comment153447>
Seems minor, but makes life easier for reviewers when code movement is pulled out in separate patches as the diff is easier to read. :)
src/linux/perf.cpp (line 417)
<https://reviews.apache.org/r/37462/#comment153442>
We already say this in the comment for this function, and it's shown in the call to split, do we need to say it again?
src/linux/perf.cpp (lines 419 - 421)
<https://reviews.apache.org/r/37462/#comment153444>
How about putting the format on a separate line as you did before?
```
// Optional running time and ratio were introduced in Linux v4.0,
// which make the format either:
// value,unit,event,cgroup
// value,unit,event,cgroup,running,ratio
```
src/linux/perf.cpp (lines 468 - 469)
<https://reviews.apache.org/r/37462/#comment153440>
It's a little bit easier to avoid missing close quotes when it's on the same line:
```
return Error("Unexpected event '" + sample->event + "'"
" in perf output at line: " + line);
```
- Ben Mahler
On Sept. 2, 2015, 6:52 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37462/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 6:52 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 bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> 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
>
>
Re: Review Request 37462: Add support for version detection and
parsing.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37462/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 6:52 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 (updated)
-----
src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
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
Re: Review Request 37462: Add support for version detection and
parsing.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
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.
Changes
-------
Rebased.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
-------
Add support for version detection and parsing.
Diffs (updated)
-----
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