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

Reply via email to