From reviews-return-53260-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Wed Jan 4 19:58:37 2017 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id C5560191A2 for ; Wed, 4 Jan 2017 19:58:37 +0000 (UTC) Received: (qmail 93852 invoked by uid 500); 4 Jan 2017 19:58:37 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 93825 invoked by uid 500); 4 Jan 2017 19:58:37 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 93808 invoked by uid 99); 4 Jan 2017 19:58:37 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 04 Jan 2017 19:58:37 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 898B2310FC5; Wed, 4 Jan 2017 19:58:36 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7698295421236738389==" MIME-Version: 1.0 Subject: Re: Review Request 54821: Refactored Docker::run() to make it only aware of docker cli options. From: Jie Yu To: Xiaojian Huang , Jie Yu , haosdent huang Cc: Zhitao Li , mesos Date: Wed, 04 Jan 2017 19:58:36 -0000 Message-ID: <20170104195836.13478.35221@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Jie Yu X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/54821/ X-Sender: Jie Yu References: <20170103174213.13569.65063@reviews.apache.org> In-Reply-To: <20170103174213.13569.65063@reviews.apache.org> Reply-To: Jie Yu X-ReviewRequest-Repository: mesos --===============7698295421236738389== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54821/#review160519 ----------------------------------------------------------- src/docker/docker.hpp (lines 137 - 145) 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) Let's add a comment for each field with the corresponding docker run option. `name` should be optional? src/docker/docker.hpp (line 149) What is this? Should that be part of `env`? src/docker/docker.hpp (line 160) `network` should be optional? src/docker/docker.hpp (line 162) We should probably introduce a Docker::PortMapping, instead of using a plain string here. src/docker/docker.hpp (line 164) Should we use `Docker::Device` here? src/docker/docker.hpp (line 184) indentation should be 4 for function parameters. src/docker/docker.cpp (line 780) s/runOptions/options/ `run` is already part of the method name. src/docker/executor.cpp (lines 172 - 181) who will trigger the shutdown of this executor in this case? src/tests/containerizer/docker_containerizer_tests.cpp (lines 1207 - 1208) 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 > > --===============7698295421236738389==--