----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29889/#review83305 -----------------------------------------------------------
src/docker/docker.cpp <https://reviews.apache.org/r/29889/#comment134286> 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. src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134292> 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. src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134245> Why can't we call 'stop.onAny(...)' directly here? src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134241> s/might still/might still be src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134242> Suggestions: Break this out as a separate statement/comment pair. s/copy to remove const/mutable copy of the future src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134294> 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. src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134247> See line 136, it seems that we do not need this function. If you were to keep it, it would be better placed above _reaped, so that one could read "upper half / lower half" contiguously. src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134248> Aren't we going to inspect 'stop'? What if it failed? Could we at least log something then? src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134295> s/after stopping/after attempting to stop src/docker/executor.cpp <https://reviews.apache.org/r/29889/#comment134296> No default? Why? src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/29889/#comment134298> Which docker image? src/slave/containerizer/docker.hpp <https://reviews.apache.org/r/29889/#comment134305> "only"? What other cases are there? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/29889/#comment134303> Which of these arguments signifies the executor? 'container'? Could we rename it accordingly? Before you updated the comment, I overlooked that this might be executor-specific code! src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/29889/#comment134310> 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. src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/29889/#comment134311> Insert blank line above, please. In other places below, too. Maybe better: move the comment above the declaration. src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/29889/#comment134314> What changed so that we don't need this test any more? - Bernd Mathiske On May 8, 2015, 2: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, 2: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 > >
