You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Cong Wang <cw...@twopensource.com> on 2015/09/03 00:14:16 UTC
Re: Review Request 37541: Add TraceEvent API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 10:14 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
Changes
-------
Rebase and cleanup
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
Diffs (updated)
-----
src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
Diff: https://reviews.apache.org/r/37541/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37541: Add trace event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/linux/perf.hpp, lines 108-110
> > <https://reviews.apache.org/r/37541/diff/9/?file=1087482#file1087482line108>
> >
> > why are these public?
Easy to access, instead of adding a get/set for each of them.
> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, lines 1125-1128
> > <https://reviews.apache.org/r/37541/diff/9/?file=1087483#file1087483line1125>
> >
> > this could use some comments.
What comments do you expect here? Some comment to explain what ID is? I thought the code is clear.
> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/tests/containerizer/cgroups_tests.cpp, line 1105
> > <https://reviews.apache.org/r/37541/diff/9/?file=1087484#file1087484line1105>
> >
> > no need for this?
We need this, otherwise we would still hold a ref to the cgroup which causes a failure to destroy it later.
> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, lines 974-1029
> > <https://reviews.apache.org/r/37541/diff/9/?file=1087483#file1087483line974>
> >
> > this could use some comments.
I will add some ASCII arts here to draw the ring buffer.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/#review107680
-----------------------------------------------------------
On Sept. 30, 2015, 12:14 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37541/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 12:14 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37541: Add trace event API
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/#review107680
-----------------------------------------------------------
src/linux/perf.hpp (line 95)
<https://reviews.apache.org/r/37541/#comment166911>
s/inline//
src/linux/perf.hpp (line 100)
<https://reviews.apache.org/r/37541/#comment166912>
s/inline//
src/linux/perf.hpp (line 102)
<https://reviews.apache.org/r/37541/#comment166914>
if you use stout's hashmap, you can do
`!fields.contains(key)`
src/linux/perf.hpp (lines 108 - 110)
<https://reviews.apache.org/r/37541/#comment166915>
why are these public?
src/linux/perf.hpp (line 114)
<https://reviews.apache.org/r/37541/#comment166913>
use stout's hashmap. it's an unordered map.
src/linux/perf.hpp (line 163)
<https://reviews.apache.org/r/37541/#comment166917>
const std::string& ?
src/linux/perf.hpp (line 165)
<https://reviews.apache.org/r/37541/#comment166916>
const Duration& ?
src/linux/perf.hpp (line 196)
<https://reviews.apache.org/r/37541/#comment166920>
s/subSys/subsystem/
src/linux/perf.hpp (line 199)
<https://reviews.apache.org/r/37541/#comment166921>
s/,/;/
s/TraceEvent's/TraceEvents/
src/linux/perf.hpp (lines 206 - 207)
<https://reviews.apache.org/r/37541/#comment166923>
s/,/;/
s/TraceEvent's/TraceEvents/
src/linux/perf.hpp (line 213)
<https://reviews.apache.org/r/37541/#comment166925>
kill extra new line.
src/linux/perf.hpp (lines 216 - 217)
<https://reviews.apache.org/r/37541/#comment166926>
don't think you need this.
src/linux/perf.hpp (lines 229 - 231)
<https://reviews.apache.org/r/37541/#comment166927>
these should be const?
src/linux/perf.cpp (line 575)
<https://reviews.apache.org/r/37541/#comment166930>
const string&
src/linux/perf.cpp (line 577)
<https://reviews.apache.org/r/37541/#comment166931>
const Duration&
src/linux/perf.cpp (line 590)
<https://reviews.apache.org/r/37541/#comment166932>
const string&
src/linux/perf.cpp (lines 920 - 931)
<https://reviews.apache.org/r/37541/#comment166933>
pull this above schedEvents to #910.
src/linux/perf.cpp (line 939)
<https://reviews.apache.org/r/37541/#comment166935>
hmm. what happens if another `parse` call comes in while the previous one is in progress?
src/linux/perf.cpp (line 951)
<https://reviews.apache.org/r/37541/#comment166934>
what if the user discards this future?
src/linux/perf.cpp (line 974)
<https://reviews.apache.org/r/37541/#comment166936>
s/ehdr/header/
or
s/ehdr/eventHeader/
src/linux/perf.cpp (lines 974 - 1029)
<https://reviews.apache.org/r/37541/#comment166937>
this could use some comments.
src/linux/perf.cpp (line 1056)
<https://reviews.apache.org/r/37541/#comment166938>
const string&
src/linux/perf.cpp (lines 1085 - 1086)
<https://reviews.apache.org/r/37541/#comment166939>
indent by 2 spaces.
src/linux/perf.cpp (line 1098)
<https://reviews.apache.org/r/37541/#comment166940>
s/subSys/subsystem/
src/linux/perf.cpp (lines 1125 - 1128)
<https://reviews.apache.org/r/37541/#comment166943>
this could use some comments.
src/linux/perf.cpp (lines 1144 - 1164)
<https://reviews.apache.org/r/37541/#comment166942>
can you add some new lines to separate blocks here. it is too dense.
src/linux/perf.cpp (lines 1209 - 1213)
<https://reviews.apache.org/r/37541/#comment166944>
why do you need to manually do this?
src/tests/containerizer/cgroups_tests.cpp (line 1093)
<https://reviews.apache.org/r/37541/#comment166946>
kill this.
src/tests/containerizer/cgroups_tests.cpp (lines 1101 - 1102)
<https://reviews.apache.org/r/37541/#comment166947>
`EXPECT_SOME(event.get("common_pid"));` ?
src/tests/containerizer/cgroups_tests.cpp (line 1105)
<https://reviews.apache.org/r/37541/#comment166948>
no need for this?
src/tests/containerizer/perf_tests.cpp (lines 205 - 206)
<https://reviews.apache.org/r/37541/#comment166949>
ditto.
- Vinod Kone
On Sept. 30, 2015, 12:14 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37541/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 12:14 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37541: Add trace event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/
-----------------------------------------------------------
(Updated Sept. 30, 2015, 12:14 a.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Cleanup and rebase
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
Diffs (updated)
-----
src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
Diff: https://reviews.apache.org/r/37541/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37541: Add trace event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/
-----------------------------------------------------------
(Updated Sept. 18, 2015, 10:12 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Rebase and cleanup
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
Diffs (updated)
-----
src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
Diff: https://reviews.apache.org/r/37541/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37541: Add trace event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/
-----------------------------------------------------------
(Updated Sept. 4, 2015, 11:15 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Rebase and cleanup
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
Diffs (updated)
-----
src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724
src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
Diff: https://reviews.apache.org/r/37541/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37541: Add trace event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37541/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 10:17 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Summary (updated)
-----------------
Add trace event API
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Based on the PerfEvent API's, add API for Linux kernel trace events, especially the schedule trace events.
Diffs
-----
src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
src/tests/containerizer/cgroups_tests.cpp 75a3bc0009c037dc18ce319db2eb44630f083e8c
src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
Diff: https://reviews.apache.org/r/37541/diff/
Testing
-------
make check
Thanks,
Cong Wang