-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44706/#review124554
-----------------------------------------------------------




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 33)
<https://reviews.apache.org/r/44706/#comment187166>

    s/like the following/as follows



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 248)
<https://reviews.apache.org/r/44706/#comment187168>

    s/i/ifindex/
    
    more descriptive.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 328 - 336)
<https://reviews.apache.org/r/44706/#comment187170>

    why do we need this dispatch ? The dispatch is to itself, so seems a bit 
odd. Can we invoke the `subprocess` for the plugin in _connect directly ?
    
    Basically not sure why we need connect & _connect.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 339)
<https://reviews.apache.org/r/44706/#comment187180>

    This is just a thought. Maybe its better to use `await` over here, and use 
a lambda or `.onAny` on the await to parse the list of futures returned on 
await and clean up any checkpointed data for networks that we were not able to 
join due to plugin errors ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 345)
<https://reviews.apache.org/r/44706/#comment187171>

    The name of the method `connect` seems a bit odd. I think you mean 
"connecting container to the network". Maybe a better name would be `attach` ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 391)
<https://reviews.apache.org/r/44706/#comment187178>

    Shouldn't we be cleaning up the check pointed data for this ifname if there 
is an error while launching the plugin for this ifname ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 445)
<https://reviews.apache.org/r/44706/#comment187172>

    Why the `CHECK_READY` here ? If the future is not READY it could be in 
error, which is fine we should just return an ERROR ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 471)
<https://reviews.apache.org/r/44706/#comment187174>

    Again why the CHECK ? The result might not have an IPv4 or an IPv6 
allocation due to an IPAM error in which case we should return a failure.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 481)
<https://reviews.apache.org/r/44706/#comment187185>

    s/write/checkpoint/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 494)
<https://reviews.apache.org/r/44706/#comment187186>

    s/write/checkpoint/


- Avinash sridharan


On March 20, 2016, 4:27 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> -----------------------------------------------------------
> 
> (Updated March 20, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
>     https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Reply via email to