> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/docker/docker.cpp, line 305 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948190#file948190line305> > > > > 1. There is no declaration in the hpp file with a comment and there is > > no explanation here. I would suggest using "static" here so nobody needs to > > browse the hpp file in search for a potential comment that does not exist. > > > > 2. OK, I can guess what this does, but future readers may want to skip > > reading the function body to find out. The name suggests running something. > > > > 3. I can't find where this function is used. > > > > It seems to me that you wanted to break out part of Docker::run() > > below, but have not removed this code there and replaced it with a call yet.
Sorry I originally made a refactor change and then abandoned it since I don't need to anymore, but thought I removed all the helper methods I added. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/docker/executor.cpp, line 139 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line139> > > > > Why can't we call 'stop.onAny(...)' directly here? Because we need to handle two cases: 1) When the docker container exits on its own 2) When the container was asked to be killed. I added a stop future because if a container was asked to be killed, the stop future holds the docker-stop future and I want to make sure I wait on that future to finish before I send back the last TASK_STATUS as this will start causing more problems in the tests where I don't know when the container will exactly stop. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/docker/executor.cpp, line 170 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line170> > > > > Could we use a LOCAL future 'stop' instead and discard 'run' once it is > > satisfied? This way 'stop' does not have to be a global and one can see > > here locally what is going on. > > > > You could then amend _reaped() for checking for run having been > > discarded. I can't, since I need to discard the docker run future first so I stop the container to be started if it wasn't launched yet. And if the container is already launched, then it will be stopped by docker-stop. If I call docker-stop first, there might be a race where we asked to the stop the container but it hasn't been launched yet, yet the docker->run future might eventually go thourgh and start the container. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/docker/executor.cpp, line 76 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line76> > > > > This is a bit tricky. You have to know that 'stop' receives a > > non-Nothing value once 'run' gets one. Suggestion: reduce this to only one > > future - 'run'. See below in line 170. See comment on line 170 comment. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/docker/executor.cpp, line 419 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line419> > > > > No default? Why? It should always be passed in, doesn't make sense to have a default in the slave flags and another default here. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/slave/containerizer/docker.hpp, line 206 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948194#file948194line206> > > > > "only"? What other cases are there? Where do you think it makes sense to specifiy all other cases? In every launch method? There are 3 paths basically, when slave is contained we launch 1) task, slave is not contained we launch 2) task or 3) custom executor. This call back is applicable for 1 and 3. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/tests/docker_containerizer_tests.cpp, line 2596 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948198#file948198line2596> > > > > What changed so that we don't need this test any more? We shouldn't need this test anymore, but thinking about it more I'll just leave it in. > On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote: > > src/tests/docker_containerizer_tests.cpp, line 175 > > <https://reviews.apache.org/r/29889/diff/9-10/?file=948198#file948198line175> > > > > We should generally avoid bool args, because it is hard to see at the > > call site what they mean. > > > > Suggestions: > > - Use an enum. > > - Keep the exists() and running() methods, factor out what's common > > between them, pass a closure as an arg to the subroutine for what's > > different. I'll just use an enum. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/#review83305 ----------------------------------------------------------- On May 8, 2015, 9:40 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29889/ > ----------------------------------------------------------- > > (Updated May 8, 2015, 9:40 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till > Toenshoff. > > > Bugs: MESOS-2115 > https://issues.apache.org/jira/browse/MESOS-2115 > > > Repository: mesos > > > Description > ------- > > This is a one mega patch and contains many reviews that's already on rb. > > This review is not meant to be merged, only provided for easier review. > > > Diffs > ----- > > Dockerfile 35abf25 > docs/configuration.md 54c4e31 > docs/docker-containerizer.md a5438b7 > src/Makefile.am 54271f7 > src/docker/docker.hpp 3ebbc1f > src/docker/docker.cpp 3a485a2 > src/docker/executor.cpp PRE-CREATION > src/slave/constants.hpp fd1c1ab > src/slave/constants.cpp 2a99b11 > src/slave/containerizer/docker.hpp b25ec55 > src/slave/containerizer/docker.cpp f9fc89a > src/slave/flags.hpp d3b1ce1 > src/slave/flags.cpp d0932b0 > src/tests/docker_containerizer_tests.cpp c9d66b3 > > Diff: https://reviews.apache.org/r/29889/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
