----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54821/#review160519 -----------------------------------------------------------
src/docker/docker.hpp (lines 137 - 145) <https://reviews.apache.org/r/54821/#comment231661> Feel that this belongs to higher level components (i.e., docker containerizer, docker executor). Can we follow with a patch to move the `create` logic to a common helper ouside this library code? src/docker/docker.hpp (lines 147 - 169) <https://reviews.apache.org/r/54821/#comment231662> Let's add a comment for each field with the corresponding docker run option. `name` should be optional? src/docker/docker.hpp (line 149) <https://reviews.apache.org/r/54821/#comment231663> What is this? Should that be part of `env`? src/docker/docker.hpp (line 160) <https://reviews.apache.org/r/54821/#comment231665> `network` should be optional? src/docker/docker.hpp (line 162) <https://reviews.apache.org/r/54821/#comment231666> We should probably introduce a Docker::PortMapping, instead of using a plain string here. src/docker/docker.hpp (line 164) <https://reviews.apache.org/r/54821/#comment231667> Should we use `Docker::Device` here? src/docker/docker.hpp (line 184) <https://reviews.apache.org/r/54821/#comment231658> indentation should be 4 for function parameters. src/docker/docker.cpp (line 780) <https://reviews.apache.org/r/54821/#comment231659> s/runOptions/options/ `run` is already part of the method name. src/docker/executor.cpp (lines 172 - 181) <https://reviews.apache.org/r/54821/#comment231675> who will trigger the shutdown of this executor in this case? src/tests/containerizer/docker_containerizer_tests.cpp (lines 1207 - 1208) <https://reviews.apache.org/r/54821/#comment231673> This fit in one line? - Jie Yu On Jan. 3, 2017, 5:42 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54821/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2017, 5:42 p.m.) > > > Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu. > > > Bugs: MESOS-6808 > https://issues.apache.org/jira/browse/MESOS-6808 > > > Repository: mesos > > > Description > ------- > > This patch creates a wrapper struct for all recognizable docker cli > options, and separate logic of creating these options to a different > common function. > > This also enables us to overcome gmock's 10 argument limit. > > No logic change happens in this refactoring patch. > > > Diffs > ----- > > src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 > src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 > src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7 > src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 > src/tests/containerizer/docker_containerizer_tests.cpp > 4e3b67bbb989f9084dfdf4970839956dcb0caa0e > src/tests/containerizer/docker_tests.cpp > 9667d434486c1832f180a297a39a3d5dae6a26bd > src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 > src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 > > Diff: https://reviews.apache.org/r/54821/diff/ > > > Testing > ------- > > `make check` with ROOT and DOCKER filter. > > > Thanks, > > Zhitao Li > >
