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