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




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

    s/netInfoDir/networkInfoDir/



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

    Can you include the 'networkInfoDir' in the error message?



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

    No need for the dispatch here. You can just do:
    ```
    foreachkey (const string& name, infos[.]->..) {
      futures.push_back(attach(containerId, name, target));
    }
    ```
    
    I would suggest we obtain NetworkInfo in 'attach' using containerId and 
name. We rarely use raw pointers.



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

    YOu can combine these two statements:
    
    ```
    return collect(futures)
      .then([]() { return Nothing(); });
    ```



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

    No need for this step. mkdir is recursive.



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

    Why do you need to get os::environment()?



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

    s/netConfig/networkConfig/



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

    We cannot read stdout and stderr after the process has finished. THis is 
becasuse the PIPE size is bounded. If the subprocess writes a lot of data, the 
write will block if there's no reader.
    
    Please follow the same pattern in:
    https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L98


- Jie Yu


On March 23, 2016, 2:22 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> -----------------------------------------------------------
> 
> (Updated March 23, 2016, 2:22 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/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Reply via email to