mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Deepak Goel <deepak.go...@gmail.com>
Subject Re: Review Request 62017: Allows port mapper plugin to have optional args.
Date Wed, 06 Sep 2017 02:46:34 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message