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

2015-09-02 Thread 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.

2015-09-02 Thread 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.

2015-09-02 Thread Ben Mahler

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


Missed this earlier, s/=//



src/linux/perf.cpp (line 287)


Looks like this comment belongs in the overload now?



src/linux/perf.cpp (lines 348 - 351)


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)


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)


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)


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)


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 


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)


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)


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)


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

2015-09-01 Thread 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.

2015-09-01 Thread 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.

2015-09-01 Thread 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.

2015-08-20 Thread 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.

2015-08-20 Thread 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.

2015-08-20 Thread 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.

2015-08-20 Thread 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