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



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

    .
    
    And can we add more documentation here to explain why you might want to set 
this to false and what the consequences are?



src/docker/docker.hpp
<https://reviews.apache.org/r/29889/#comment132143>

    While you're updating this line, s/> >/>>/ please.



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

    s/!//



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

    s/if(/if (/
    
    Also, let's pull this out as a constant for now, with a TODO to make it a 
flag.



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

    s/.//
    
    Here and everywhere else.



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

    Why not just make 'run' be the class member field?



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

    Why not CopyFrom?



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

    Please add a comment for why you're doing this, and I prefer this instead:
    
    // Comment here why need to make copy.
    Future<Nothing>(run.get()).discard();



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

    s/id/ID/
    
    Here and everywhere else please.



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

    Comment me!!
    
    Actually, this looks like dead code. ;-)



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

    s/still to/still have to/



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

    Try<Docker*> create = Docker::create(flags.docker);
    if (create.isError()) {
      return Error("Failed to create Docker: " + create.error());
    }
    
    Shared<Docker> docker(create.get());


- Benjamin Hindman


On April 22, 2015, 10: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, 10: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