This is an automatically generated e-mail. To reply, visit:

Ship it!


    s/slave recovers/slave restarts and recovers/


    Can you add a small comment explaining when 'started' would be 'false'? I'm 
guessing you want to use 'started' here to help figure out when a container has 
"started" when you do an attached 'docker run', but I don't know what 'started' 
implies, and how it is different than checking when 'pid.isSome()'?


    FYI, there have been additions to the JSON library that should make this 
(and the code above) much easier now. In particular, you should now be able to 
    Result<JSON::String> startedAt = 
    if (startedAt.isError()) {
      return Error("Unable to find 'StartedAt' in State: " + startedAt.error());
    } else if (startedAt.isError()) {
      return Error("No 'StartedAt' found in State");
    bool started = startedAt.value != "0001-01-01T00:00:00Z";
    This cleanup needs to happen across this entire file so it might make sense 
to just put a TODO here for now if you don't want to take this on just yet.


    Any reason not to make these Option as well? It seems a little wierd to 
potentially read these and get garbage (or missing) data in the case that they 
were actually passed into the constructor as 'None'.


    Not used? Looks shadowed below.


    Super duper clean Tim!


    Yes Yes Yes YES YES!


    Why do you need to associate the promise within the 'onAny' callback? You 
should just be able to do:
    Future<Docker::Container> inspect =
        docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY);
    The other thing that is a bit mysterious to me here is why you actually 
want/need an extra Promise? Seems like you can just do:
    run.onFailed([=](const string& failure) {
    return inspect;
    Maybe it's because you don't want a 'discarded' future to propagate out 
from 'launchExecutorContainer'? In which case, I'd comment as much, i.e., that 
you explicitly want a failed future rather than let a discarded future 
propagate and clearly explain why (hopefully there isn't other code relying on 
it someplace because that's definitely a global invariant that we should 


    So this version is correctly working? Just wanted to call this out 
explicitly because the other one is not and that makes me wonder if maybe this 
code path isn't being tested?


    Wait, are we doing a 'docker rm -f' here or just a 'docker stop'?


    I don't fully grok this, does this mean that we always have two containers? 
I would have expected a version of remove that maybe takes an 'Option<string> 
executorContainerName' and only does an two 'docker rm' if there are two 
containers. Were you just assuming that the extra 'docker rm' would be a no-op? 
If so, you should definitely document that, but I'm also not convinced that 
it's not hard for us to know whether or not we need to do two 'docker rm' 
calls, or am I missing something?


    See comment above!


    This looks like we're introducing a broken window. Maybe you were looking 
    AWAIT_READY_TRUE(docker.get()->rm(container.id, true), Seconds(30));
    And that didn't exist? We should create it! In the mean time, I'd rather 
    AWAIT_READY_EQ(true, docker.get()->rm(container.id, true), Seconds(30));
    Then let's get an AWAIT_READY_TRUE implemented later! I can help, just let 
me know!


    Looks like we need another helper in libprocess! Are you looking for 
something along the lines of:
    Can you follow up with me on all of these kinds of helpers that you're 
missing? Adding in await everywhere is a pattern I'd like to avoid.

- Benjamin Hindman

On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
> (Updated May 23, 2015, 6:14 a.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 34755cf 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.hpp PRE-CREATION 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/constants.hpp fd1c1ab 
>   src/slave/constants.cpp 2a99b11 
>   src/slave/containerizer/docker.hpp ed4ee19 
>   src/slave/containerizer/docker.cpp 408a443 
>   src/slave/flags.hpp 5c57478 
>   src/slave/flags.cpp b5e2518 
>   src/tests/docker_containerizer_tests.cpp 154bf98 
>   src/tests/docker_tests.cpp 5520c58 
> Diff: https://reviews.apache.org/r/29889/diff/
> Testing
> -------
> make check
> Thanks,
> Timothy Chen

Reply via email to