Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Ben Mahler]

---
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)


Why not make this an Option and when it's  this the 
value is None?



src/linux/perf.cpp (line 498)


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)


Indentation is off here, please have a look over the diff, also s/if(/if (/ 
:)



src/linux/perf.cpp (lines 501 - 504)


Don't forget to update this when you rebase.



src/linux/perf.cpp (line 511)


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)


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)


Can you now use the -> operator here? Might also avoid the need to wrap 
these, although, the "" logic should be moved to 'parse()'.



src/linux/perf.cpp (lines 540 - 543)


Now that you have a parse function, let's put the  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
> 
>

Re: Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Paul Brett]

---
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.

[Paul Brett]

---
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.

[Ben Mahler]

---
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)


This looks like the documentation for 'parse'?



src/linux/perf.cpp (lines 392 - 394)


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.

[Paul Brett]

> On Sept. 1, 2015, 7:09 p.m., Ben Mahler wrote:
> > src/linux/perf.cpp, line 490
> > 
> >
> > Why not make this an Option and when it's  this 
> > the value is None?

Possible values are uint64_t, double, "" and "".  
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.

[Paul Brett]

---
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.

[Paul Brett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/
---

(Updated Aug. 20, 2015, 5:31 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Remove redundant constructor.


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

Review Request 37442: Factor out the token extraction rules in prepartion for extending them to cope with multiple versions.

[Paul Brett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/
---

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 cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 

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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37442/#review95371
---


Patch looks great!

Reviews applied: [37423, 37424, 37417, 37416, 37442]

All tests passed.

- Mesos ReviewBot


On Aug. 13, 2015, 9:59 p.m., Paul Brett wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37442/
 ---
 
 (Updated Aug. 13, 2015, 9:59 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 cb1a13d0b1754a50f0121bfda522056ff8c3e3c8 
 
 Diff: https://reviews.apache.org/r/37442/diff/
 
 
 Testing
 ---
 
 sudo make check
 
 
 Thanks,
 
 Paul Brett