This is an automatically generated e-mail. To reply, visit:

src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 103)

    Why do we need to use a hashmap over here? Why not a vector or a list? 
Hashmap's are expensive in terms of memory. Usually a container won't be 
associated with more than one network, so seems pretty inefficient?

src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 139)

    Maybe s/recoverInfo/_recover/

src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 151)

    Should have pointed in the earlier patches, why do we need `Info` to be a 
smart pointer. We can have it as an object. We are not going to share this 
information with any other class or thread.

src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 136)

    Why do we have this change as part of this patch? Can we move this out to a 
different patch?

src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 288 - 290)

    Do we need this loop? `infos.clear` below should destroy the hashmap which 
in turn would destroy any of the residing objects (`networkinfos`)


    I think we should have this in a separate patch?

src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 809)

    Isn't this a problem? We are returning `Nothing` here, which implies that 
in the `recover` method the container information is assumed to have been 
stored correctly. However, there is NO container informationt that has been 
stored!! I thought we should never run into this issue, since we should be 
creating the directories before calling the CNI plugin?

src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 824 - 827)

    What if the isolator dies right after creating the directories, but after 
invoking the plugin? If we just return `Nothing` we might end up leaking state 
information from the CNI plugin (IPAM). I think we should return `Error` here 
and ask the isolator to cleanup the containers and start over.

src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 870)

    This error doesn't make sense. Maybe:
    "Found multiple interfaces for a given CNI network. This should not be 
allowed" ?

src/slave/containerizer/mesos/isolators/network/cni/spec.cpp (line 53)

    Why not just return `parse` ?

- Avinash sridharan

On March 29, 2016, 11 a.m., Qian Zhang wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45383/
> -----------------------------------------------------------
> (Updated March 29, 2016, 11 a.m.)
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> Repository: mesos
> Description
> -------
> Implemented recover() method of "network/cni" isolator.
> Diffs
> -----
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 6a3c33645bab73edaf5af4d298a671852ea59c46 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> 5b5f904def9ef6dcc4462a03a2d024ad4eb3d946 
> Diff: https://reviews.apache.org/r/45383/diff/
> Testing
> -------
> Thanks,
> Qian Zhang

Reply via email to