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

Reply via email to