----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59294/#review175077 -----------------------------------------------------------
Looks good overall. Just a few comments. src/slave/containerizer/mesos/isolators/network/port_mapping.hpp Lines 323 (patched) <https://reviews.apache.org/r/59294/#comment248423> Make it a `const` method? src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 696-697 (patched) <https://reviews.apache.org/r/59294/#comment248426> Maybe add a note here that the logic below is guarded by this check, so someone won't accidently "simplify" it? src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Line 1589 (original), 1715-1716 (patched) <https://reviews.apache.org/r/59294/#comment248427> Should we also add a check that `minimum_egress_rate_limit <= maximum_egress_rate_limit`? If that's not true egress rate limit will always be set to `maximum_egress_rate_limit`, even if originally it was smaller. What if `speed < maximum_egress_rate_limit`? Should we provide a warning? src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 2176 (patched) <https://reviews.apache.org/r/59294/#comment248429> `strings::remove(handle, "0", strings::SUFFIX)`? We use "1:0" handle, but just in case. src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 2679-2681 (patched) <https://reviews.apache.org/r/59294/#comment248438> Should we also log this when no ephemeral ports were found for the container? src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 2680-2681 (patched) <https://reviews.apache.org/r/59294/#comment248439> `Try::operator->` can be used instead of `Try::get()`. src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 4177-4178 (patched) <https://reviews.apache.org/r/59294/#comment248443> What if a container uses `cpus:0.5` and `minimum_egress_rate_limit` is unset? Shouldn't `limit` have some minimum resonable value instead of 0? src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 4184 (patched) <https://reviews.apache.org/r/59294/#comment248446> There's no need to use `Bytes()` constructor here. The result of `max()` here is already `Bytes`. src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 4188 (patched) <https://reviews.apache.org/r/59294/#comment248447> Ditto. - Ilya Pronin On May 15, 2017, 9:56 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59294/ > ----------------------------------------------------------- > > (Updated May 15, 2017, 9:56 p.m.) > > > Review request for mesos, Dmitry Zhuk, Ilya Pronin, Jie Yu, and Santhosh > Kumar Shanmugham. > > > Bugs: MESOS-7508 > https://issues.apache.org/jira/browse/MESOS-7508 > > > Repository: mesos > > > Description > ------- > > Add support to isolators/port_mapping for optionally scaling egress bandwidth > with CPU and with minimum and maximum limits. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/port_mapping.hpp > 9d38289c7161d5e931053b587d115684ccc44c94 > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > cd008aaebcd42554a9a81d2b059269546f59c966 > src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd > src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c > src/tests/containerizer/port_mapping_tests.cpp > a528382e8b4831b9c7e8dcc877a5e242909f0cd5 > > > Diff: https://reviews.apache.org/r/59294/diff/1/ > > > Testing > ------- > > # added a new test > $ make check > > > Thanks, > > Ian Downes > >
