> On Sept. 6, 2017, 2:09 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > > Lines 149 (patched) > > <https://reviews.apache.org/r/62017/diff/4/?file=1816054#file1816054line149> > > > > Why are we setting this empty `args` here instead of setting up a > > `NetworkInfo networkInfo` object and passing it down into the constructor > > of the port-mapper? > > > > the `port-mapper` really doesn't care about `args` it cares about > > `NetworkInfo`. This code seems to conflate the whole need for not setting > > up `args` here. > > > > Also setting up `mesos.values` seems to tie the port-mapper plugin back > > to Mesos semantics of setting up the args which seems like a bit of a hack.
Yeah, you are right. Initially, I thought of doing that but then it led to making exceptions to various checks that happens before we reach to the constructor and the code looked even more hacky and error prone. This patch achieves the same goal with fewer lines of code change - Deepak ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62017/#review184620 ----------------------------------------------------------- On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62017/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2017, 12:25 a.m.) > > > Review request for mesos and Avinash sridharan. > > > Bugs: mesos-7923 > https://issues.apache.org/jira/browse/mesos-7923 > > > Repository: mesos > > > Description > ------- > > Mesos port mapper cni plugin is a wrapper around bridge plugin > to add port mapping functionality to bridge plugin. However, in > certain cases the network creator doesn't need port mapping > functionality and just want to access bridge plugin. In this case, > the creator may not supply `args` in cni config which will makes > mesos port mapper plugin to fail. This patch makes `args` in cni > config optional for mesos port mapper plugin > > > Diffs > ----- > > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 > > > Diff: https://reviews.apache.org/r/62017/diff/4/ > > > Testing > ------- > > > Thanks, > > Deepak Goel > >
