----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26090/#review54706 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/26090/#comment94973> s/net_rtt_median/net_tcp_rtt_median/ include/mesos/mesos.proto <https://reviews.apache.org/r/26090/#comment94978> Let's not expose it for now. src/slave/containerizer/isolators/network/helper.cpp <https://reviews.apache.org/r/26090/#comment94974> s/PortMappingUsage/PortMappingStatistics/ src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/26090/#comment94979> Reorder these two. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94977> s/PortMappingUsage/PortMappingStatistics/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94981> s/usage/statistics/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94990> I prefer not to change here. I prefer simpler semantics: if anything goes wrong, return a failure. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94982> Why PIPE for input? Use /dev/null src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94983> Do you care about error message? I should suggest to use /dev/null as well here. Checking exit code should be sufficient. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94991> See my above comments. if io::read fails, you return failure without returning the rx_* stats? Therefore, use a simpler semantics. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94984> We usually put `lambda::_1` at the end of the argument list. I would suggest this order: ``` containerId, result, s.get(), lambda::_1 ``` src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94985> Failed to read the socket statistics for container ... src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94986> Move this comment down above JSON::parse src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94987> What if the subprocess is killed in the middle of writing output? Do not use CHECK here. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94988> Do not use CHECK. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94992> This is a little weired, but I understand your concern. I would suggest checking status first and add a TODO about the blocking pipe. I don't think this is a problem right now, so let's punk that for now. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94989> Reorder those arguments. See my above comments. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94993> No need to the the stderr. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/26090/#comment94994> This is spammy, remove it - Jie Yu On Sept. 26, 2014, 6:05 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26090/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2014, 6:05 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. > > > Bugs: mesos-1808 > https://issues.apache.org/jira/browse/mesos-1808 > > > Repository: mesos-git > > > Description > ------- > > see summary. > > > Diffs > ----- > > include/mesos/mesos.proto be45494 > src/slave/containerizer/isolators/network/helper.cpp 6cbcb33 > src/slave/containerizer/isolators/network/port_mapping.hpp b624c4d > src/slave/containerizer/isolators/network/port_mapping.cpp 2766a00 > > Diff: https://reviews.apache.org/r/26090/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
