----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32660/#review81575 -----------------------------------------------------------
maybe pull out the improvment part to existing code. makes that part quicker to go in and this one shorter and easier to review. include/mesos/mesos.proto <https://reviews.apache.org/r/32660/#comment131993> let's avoid a new term network rate limiter and just use BW capping? include/mesos/mesos.proto <https://reviews.apache.org/r/32660/#comment131994> maybe make it clear that delayed maps to capping; dropped maps to NIC capacity exceeded? include/mesos/mesos.proto <https://reviews.apache.org/r/32660/#comment131995> Maybe give a reference link to where these are introduced? I am a bit confused about backlog and qlen include/mesos/mesos.proto <https://reviews.apache.org/r/32660/#comment131996> required makes it hard to update a protobuf: https://developers.google.com/protocol-buffers/docs/proto#updating include/mesos/mesos.proto <https://reviews.apache.org/r/32660/#comment131997> this comment can probably be saved. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment131998> is this used? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment131999> comment doesn't match src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132007> auto isn't whitelisted yet. here and everywhere. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132013> is this c++11? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132001> else use error_message? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132002> use error_message for errors? here and everywhere. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132008> should 'net_stats' be in a constant as well? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132003> personally i think just setting them directly is the easist to understand. see comment below. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132009> ditto. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132010> s/str/id/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132000> Does it de-duplicate code? it isn't as easy to read as before to me, only to save the 'if (metric.isSome())' part src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132004> mind explaining why you call them limiter and fairshare? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132011> also check and output error_message here? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132005> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32660/#comment132006> :) - Chi Zhang On April 24, 2015, 7:42 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32660/ > ----------------------------------------------------------- > > (Updated April 24, 2015, 7:42 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. > > > Bugs: mesos-2332 > https://issues.apache.org/jira/browse/mesos-2332 > > > Repository: mesos > > > Description > ------- > > Report network isolator statistics on a per container basis (MESOS-2332) > > > Diffs > ----- > > include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 > src/slave/containerizer/isolators/network/port_mapping.hpp > 466cd82e69665af217d61392b739b9bba16e1e13 > src/slave/containerizer/isolators/network/port_mapping.cpp > ccdc44f465f204f674b859c429ba1a6ada51cd88 > src/slave/containerizer/mesos/containerizer.hpp > fb334e53bc8d73bced402095daddab5e06dc2dd1 > src/slave/containerizer/mesos/containerizer.cpp > ea3b499258b6be4a643d246159a17a0eaeedf904 > > Diff: https://reviews.apache.org/r/32660/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Paul Brett > >
