-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review81873
-----------------------------------------------------------



Dockerfile
<https://reviews.apache.org/r/29889/#comment132377>

    It seems that docker.io could be on its own line, since it is a kinda 
separate topic, which seems to be the grouping criterium here?



docs/configuration.md
<https://reviews.apache.org/r/29889/#comment132378>

    s/CLI/Docker client CLI/



docs/configuration.md
<https://reviews.apache.org/r/29889/#comment132385>

    Suggestion how to phrase all this:
    
    The UNIX socket path to be mounted into the docker executor container to 
provide docker CLI access to the docker daemon. This must be the path used by 
the slave's docker image.



docs/configuration.md
<https://reviews.apache.org/r/29889/#comment132387>

    s/a/an



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132389>

    s/!isSome/isNone



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132390>

    s/> >/>>



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132391>

    s/contianer/container



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132394>

    Unfortunately, checkError() and _run() are so similar, but so differently 
named. Maybe a comment explaining that error checking is different when not 
detached in that stderr does not get read?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132400>

    Suggestion to replace the last sentence with: 
    
    The executor is set up in a way that ensures its continued execution if the 
slave exits. If the slave is running in a docker container this is inherently 
the case. Otherwise the executor creates a new process group for itself.



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132402>

    Shouldn't we leave a trace of what happened in the output?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132403>

    Given rephrasing the class comment above, this comment may get removed.



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132406>

    Do we really need this variable? Could we just use task.container()?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132410>

    Please comment why we are adding the executor resources here: for reporting 
total usage of task + executor instead of just task. However, isn't it the case 
that we are now actually using "task.resources() + executor.resources() + 
executor.resources()", since the xecutor is already running?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132412>

    Out of the 3 vars "launched", "dockerRun", "killed" you only need 2.



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132413>

    This is also virtual in the CommandExecutor, but why?


- Bernd Mathiske


On April 22, 2015, 3:50 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 3:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> 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 93c7c8a 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/containerizer/docker.hpp 0d5289c 
>   src/slave/containerizer/docker.cpp f9fb078 
>   src/slave/flags.hpp d3b1ce1 
>   src/slave/flags.cpp d0932b0 
>   src/tests/docker_containerizer_tests.cpp b119a17 
> 
> Diff: https://reviews.apache.org/r/29889/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Reply via email to