----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38117/#review102933 -----------------------------------------------------------
include/mesos/mesos.proto (lines 702 - 720) <https://reviews.apache.org/r/38117/#comment160766> Are the statistics signed? If not, suggest using uint64 type. src/slave/containerizer/isolators/network/port_mapping.cpp (line 1139) <https://reviews.apache.org/r/38117/#comment160779> No need to store all statistics until the end. Key/value lines for a set of statistics are paired so you can add*Statistics after processing each line pair. Then you just need a scoped {{hashmap<string, int64_t> statistics}}. src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1140 - 1141) <https://reviews.apache.org/r/38117/#comment160778> Clearer to just declare these inside the loop scope (but my suggestions below obviate them anyway). src/slave/containerizer/isolators/network/port_mapping.cpp (line 1142) <https://reviews.apache.org/r/38117/#comment160777> you don't care about the actual line number, just that you want to toggle between "key" and "value" lines, suggest something like "bool isKeyLine" and toggle that? src/slave/containerizer/isolators/network/port_mapping.cpp (line 1144) <https://reviews.apache.org/r/38117/#comment160770> Suggest splitting first on ":" to get the statistics type and keys/values, then tokenize on " ". You can also validate the type then as well. src/slave/containerizer/isolators/network/port_mapping.cpp (line 1146) <https://reviews.apache.org/r/38117/#comment160774> If you split first on ":" then you don't need a separate keys vector. src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1150 - 1159) <https://reviews.apache.org/r/38117/#comment160775> Assuming you've got the type from splitting on ":" and then you've got the keys, you can directly insert val into a statistics hashmap after successfully numifying it, i.e., no need for an intermediate values vector. src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1161 - 1165) <https://reviews.apache.org/r/38117/#comment160780> This is an unnecessary mix of indexing and foreach looping. Comments above make this unnecessary but just FYI. src/slave/containerizer/isolators/network/port_mapping.cpp (line 1162) <https://reviews.apache.org/r/38117/#comment160783> This should be a {{const string&}} (if kept). src/slave/containerizer/isolators/network/port_mapping.cpp (line 1169) <https://reviews.apache.org/r/38117/#comment160782> Toggle line type here. src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1171 - 1174) <https://reviews.apache.org/r/38117/#comment160781> Do these inside the parsing loop as you parse each line pair for a statistics type. - Ian Downes On Sept. 8, 2015, 2:12 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38117/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2015, 2:12 p.m.) > > > Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone. > > > Bugs: MESOS-3365 > https://issues.apache.org/jira/browse/MESOS-3365 > > > Repository: mesos > > > Description > ------- > > These stats are those we get by `netstat -s`, they are important for diagnose > networking issues. > > > Diffs > ----- > > include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c > src/slave/containerizer/isolators/network/port_mapping.hpp > 4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 > src/slave/containerizer/isolators/network/port_mapping.cpp > 34ba2294b0bd7d57aa9de073692a2ea8ec62681d > src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c > src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d > > Diff: https://reviews.apache.org/r/38117/diff/ > > > Testing > ------- > > Manual tests > > > Thanks, > > Cong Wang > >
