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