Re: Review Request 37462: Add support for version detection and parsing.

[Paul Brett]

> On Sept. 1, 2015, 11:43 p.m., Ben Mahler wrote:
> > src/linux/perf.hpp, line 56
> > 
> >
> > 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 c5630118f4858b1a805f25a2db7a24ca0989 
>   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.

[Paul Brett]

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

[Paul Brett]

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

[Ben Mahler]

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


Whoops, doesn't this comment belong on the parse function?



src/linux/perf.cpp (lines 278 - 281)


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)


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 output = perf->output();
spawn(perf, true);

auto parse = [start, duration](
const Version& version,
const string& output) ->
Future> {
  Try> 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)


Let's just make an overload of supported that takes a version, see my 
comment above.



src/linux/perf.cpp (line 408)


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)


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)


Braces please :)

Also, this can just be a single statement?

```
if (tokens.size() == 4 || tokens.size() = 6) {
  ...
}
```



src/linux/perf.cpp (lines 422 - 423)


Braces please :)



src/linux/perf.cpp (lines 427 - 428)


Braces please :)



src/linux/perf.cpp (line 430)


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)


Do you have a tool that is removing spaces on if statements, or?



src/tests/containerizer/perf_tests.cpp 


Why did this get removed?



src/tests/containerizer/perf_tests.cpp (line 69)


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

Re: Review Request 37462: Add support for version detection and parsing.

[Paul Brett]

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

[Paul Brett]

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

(Updated Aug. 21, 2015, 12:48 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebase.


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

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.

[Paul Brett]

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

(Updated Aug. 20, 2015, 11:49 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Fix handling of empty fields.


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

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.

[Paul Brett]

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

(Updated Aug. 21, 2015, midnight)


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

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.

[Paul Brett]

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

(Updated Aug. 20, 2015, 7:17 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Improve readability for extract function.


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

Diff: https://reviews.apache.org/r/37462/diff/


Testing (updated)
---

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

Review Request 37462: Add support for version detection and parsing.

[Paul Brett]

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

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

Diff: https://reviews.apache.org/r/37462/diff/


Testing
---

sudo make check


Thanks,

Paul Brett