You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/02/15 23:22:21 UTC
Review Request 56732: Remove unnecessary perf version checks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56732/
-----------------------------------------------------------
Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
Bugs: MESOS-7049
https://issues.apache.org/jira/browse/MESOS-7049
Repository: mesos
Description
-------
We can remove the version check from the perf subprocess
wrapper because we longer uses the version to figure out
how to parse the stat output and the callers explicitly
check perf::supported().
Diffs
-----
src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
Diff: https://reviews.apache.org/r/56732/diff/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56732: Remove unnecessary perf version checks.
Posted by haosdent huang <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56732/#review168234
-----------------------------------------------------------
Ship it!
Ship It!
- haosdent huang
On Feb. 15, 2017, 11:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56732/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 11:22 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We can remove the version check from the perf subprocess
> wrapper because we longer uses the version to figure out
> how to parse the stat output and the callers explicitly
> check perf::supported().
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
>
> Diff: https://reviews.apache.org/r/56732/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56732: Remove unnecessary perf version checks.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On April 28, 2017, 4:06 p.m., Jiang Yan Xu wrote:
> > src/linux/perf.cpp
> > Line 322 (original), 313 (patched)
> > <https://reviews.apache.org/r/56732/diff/1/?file=1635867#file1635867line322>
> >
> > Now without the `process::collect`.
> >
> > 1. Remove from includes:
> > ```
> > #include <process/collect.hpp>
> > ```
> >
> > 2. We can just inline the lambda (the way we usually do this)
> >
> > ```
> > return output
> > .then([start, duration](const string output)
> > -> Future<hashmap<string, mesos::PerfStatistics>> {
> > Try<hashmap<string, mesos::PerfStatistics>> result =
> > perf::parse(output);
> >
> > 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();
> > });
> > ```
Apparently we have to keep `#include <process/collect.hpp>` for `process:await`.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56732/#review173393
-----------------------------------------------------------
On Feb. 15, 2017, 3:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56732/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 3:22 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We can remove the version check from the perf subprocess
> wrapper because we longer uses the version to figure out
> how to parse the stat output and the callers explicitly
> check perf::supported().
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
>
> Diff: https://reviews.apache.org/r/56732/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56732: Remove unnecessary perf version checks.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56732/#review173393
-----------------------------------------------------------
Fix it, then Ship it!
Committing with the minor changes if no objection.
src/linux/perf.cpp
Line 322 (original), 313 (patched)
<https://reviews.apache.org/r/56732/#comment246394>
Now without the `process::collect`.
1. Remove from includes:
```
#include <process/collect.hpp>
```
2. We can just inline the lambda (the way we usually do this)
```
return output
.then([start, duration](const string output)
-> Future<hashmap<string, mesos::PerfStatistics>> {
Try<hashmap<string, mesos::PerfStatistics>> result =
perf::parse(output);
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();
});
```
- Jiang Yan Xu
On Feb. 15, 2017, 3:22 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56732/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 3:22 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We can remove the version check from the perf subprocess
> wrapper because we longer uses the version to figure out
> how to parse the stat output and the callers explicitly
> check perf::supported().
>
>
> Diffs
> -----
>
> src/linux/perf.hpp 9c4330b6086abb18f036222260fe89a6fb01d8ed
> src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb
> src/tests/containerizer/perf_tests.cpp d536ecc5cb24787bc3487efb146573cd4f3ded43
>
>
> Diff: https://reviews.apache.org/r/56732/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>