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



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

    Can we add some output here, too, indicating that the reason for shutting 
down is killing?



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

    Emit some output here?



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

    This is copied from CommandExecutor. Please add a TODO to fix both.



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

    Owned<...>



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

    cli -> executable



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

    Suggestion: 
    "The path to the container sandbox holding stdout and stderr files into 
which docker container logs will be redirected."



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

    This pertains to the boolean arg. Please indicate this.



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

    Suggestion, add something like: "..., since this has already been validated 
in <which step> before."



src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29889/#comment133026>

    This is a good place to explain the whole picture what each override<...> 
is, what it's purpose is and how it gets used. However, if this is deemed a 
generic facility, then some of the use sites need to be upgraded with 
docker-specific scenario descriptions.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133027>

    zap the TABs



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133028>

    You have a constant declared for 5.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133029>

    Would it be possible to pull up the conditional branch to here and then 
call two different constructors depending on the condition, each of which fills 
out the fields differently. We would then no longer need the override<...> 
fields, would we?



src/slave/flags.cpp
<https://reviews.apache.org/r/29889/#comment133021>

    There is extra text on this in the docs. Please determine how much of it 
pull up here as well.


- Bernd Mathiske


On May 1, 2015, 2:43 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
> 
> (Updated May 1, 2015, 2:43 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 93c7c8a 
>   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