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 00:33:13 UTC
Re: Review Request 37442: Factor out the token extraction rules in
prepartion for extending them to cope with multiple versions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/
-----------------------------------------------------------
(Updated Aug. 31, 2015, 10:33 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Rebased
Repository: mesos
Description
-------
Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.
Diffs (updated)
-----
src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78
Diff: https://reviews.apache.org/r/37442/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37442: Factor out the token extraction rules in
prepartion for extending them to cope with multiple versions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/#review97359
-----------------------------------------------------------
Ship it!
Will get this committed shortly.
src/linux/perf.cpp (lines 381 - 382)
<https://reviews.apache.org/r/37442/#comment153233>
This looks like the documentation for 'parse'?
src/linux/perf.cpp (lines 392 - 394)
<https://reviews.apache.org/r/37442/#comment153232>
Either? :)
- Ben Mahler
On Sept. 1, 2015, 8:42 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37442/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 8:42 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee
>
> Diff: https://reviews.apache.org/r/37442/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37442: Factor out the token extraction rules in
prepartion for extending them to cope with multiple versions.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 8:42 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Incorporate some of Ben's feedback. Does not implement decoding of the Sample value field in Sample::parse.
Repository: mesos
Description
-------
Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.
Diffs (updated)
-----
src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee
Diff: https://reviews.apache.org/r/37442/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37442: Factor out the token extraction rules in
prepartion for extending them to cope with multiple versions.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 7:49 p.m.)
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.
Diffs (updated)
-----
src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee
Diff: https://reviews.apache.org/r/37442/diff/
Testing
-------
sudo make check
Thanks,
Paul Brett
Re: Review Request 37442: Factor out the token extraction rules in
prepartion for extending them to cope with multiple versions.
Posted by Paul Brett <pa...@twopensource.com>.
> On Sept. 1, 2015, 7:09 p.m., Ben Mahler wrote:
> > src/linux/perf.cpp, line 490
> > <https://reviews.apache.org/r/37442/diff/3/?file=1060677#file1060677line490>
> >
> > Why not make this an Option<uint64_t> and when it's <not counted> this the value is None?
Possible values are uint64_t, double, "<not counted>" and "<not supported>". I'm not sure how useful it is to distingish between the last two but choosing between uint64_t and double cannot be done until we lookup the event.
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/#review97320
-----------------------------------------------------------
On Sept. 1, 2015, 7:49 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37442/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 7:49 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp dac7061471a0fa05de12cb530bcd5c63a6a71eee
>
> Diff: https://reviews.apache.org/r/37442/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>
Re: Review Request 37442: Factor out the token extraction rules in
prepartion for extending them to cope with multiple versions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/#review97320
-----------------------------------------------------------
Some notes before you rebase.
src/linux/perf.cpp (line 490)
<https://reviews.apache.org/r/37442/#comment153163>
Why not make this an Option<uint64_t> and when it's <not counted> this the value is None?
src/linux/perf.cpp (line 498)
<https://reviews.apache.org/r/37442/#comment153164>
How about 'parse' to be consistent with how we've named these kinds of functions, perhaps making it a static member of Sample?
No need for inline here?
src/linux/perf.cpp (lines 500 - 512)
<https://reviews.apache.org/r/37442/#comment153165>
Indentation is off here, please have a look over the diff, also s/if(/if (/ :)
src/linux/perf.cpp (lines 501 - 504)
<https://reviews.apache.org/r/37442/#comment153166>
Don't forget to update this when you rebase.
src/linux/perf.cpp (line 511)
<https://reviews.apache.org/r/37442/#comment153168>
The caller is expected to print the line when composing the error message, the callee here should print why the line is bad.
src/linux/perf.cpp (lines 522 - 524)
<https://reviews.apache.org/r/37442/#comment153169>
Let's include the line contents in the error message by composing a message with the parse error:
```
return Error("Failed to parse perf sample line '" + line + "': " + parse.error());
```
src/linux/perf.cpp (lines 531 - 577)
<https://reviews.apache.org/r/37442/#comment153170>
Can you now use the -> operator here? Might also avoid the need to wrap these, although, the "<not counted>" logic should be moved to 'parse()'.
src/linux/perf.cpp (lines 540 - 543)
<https://reviews.apache.org/r/37442/#comment153171>
Now that you have a parse function, let's put the <not supported> logic there as well.
- Ben Mahler
On Aug. 31, 2015, 10:33 p.m., Paul Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37442/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2015, 10:33 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78
>
> Diff: https://reviews.apache.org/r/37442/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Paul Brett
>
>