mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 24475: Add new Docker configurations
Date Mon, 11 Aug 2014 04:34:57 GMT

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


The biggest thing this review could use is some tests which exercise the new fetching control
flows in the DockerContainerizer. What happens when fetching fails?

Otherwise, looking pretty good!


include/mesos/mesos.proto
<https://reviews.apache.org/r/24475/#comment87719>

    Please move this just below 'command' and also add a comment above both of them that explains
the semantics. Same thing in ExecutorInfo too please!



include/mesos/mesos.proto
<https://reviews.apache.org/r/24475/#comment87720>

    Why 100?



src/docker/docker.cpp
<https://reviews.apache.org/r/24475/#comment87721>

    We'll likely want to make this a flag.



src/docker/docker.cpp
<https://reviews.apache.org/r/24475/#comment87722>

    s/var/variable/ ;-)



src/docker/docker.cpp
<https://reviews.apache.org/r/24475/#comment87723>

    Was there something more you wanted to say here? ;-)



src/docker/docker.cpp
<https://reviews.apache.org/r/24475/#comment87724>

    s/runEnv/environment/



src/docker/docker.cpp
<https://reviews.apache.org/r/24475/#comment87725>

    Can you add to the comment for posterity why you didn't just cd into the directory when
you ran this command?



src/slave/containerizer/containerizer.hpp
<https://reviews.apache.org/r/24475/#comment87726>

    Add newline please.



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

    Can we use the continuation syntax to name this method? It'll make it easier for someone
to do a broad sweep and add C++11 lambdas later.



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

    It doesn't look like you're every putting containers into 'fetching'! Also, this is starting
to get a lot more complicated, so I'd suggest adding both comments and CHECKs as much as possible
(a CHECK would have caught your bug!). As in, IIUC, now you're expecting that 'fetching' and
'promises' are disjoint, and that a container "transitions" through from "fetching" to "launched",
i.e., from the 'fetching' set to the 'promises' set. Am I correct? Let's make it easier for
other people to understand these semantics.
    
    Also, I'm assuming that you didn't want to just put the container in 'promises' until
after fetching is complete because it was easier to "cleanup" after fetching (whether it's
successful or an failure) by just removing from 'fetching', but I wanted to mention that you
can use .onFailed to cleanup 'promises' if fetching fails and otherwise just fall through
to the next continuation where 'promises' still has the container.



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

    Proper capitalization please: 'URIs' over 'uris'.



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

    Can you put the .then on the next line? It's easier to read and becoming more standard
in the code base.



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

    Please comment on MESOS-1686 since it was addressing this code that you're now killing.



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

    Just re-iterating what I was commented on earlier, if you put the container in 'promises'
then you could do the following here (and below in the other 'launch' too):
    
    fetch(...)
      .onFailed([] () { promises.erase(containerId); })
      .then(... _launch ...);
    
    Maybe I'm missing something else? If so, need more documentation please!



src/slave/slave.cpp
<https://reviews.apache.org/r/24475/#comment87739>

    I thought the plan was to force CommandInfo even if ContainerInfo is present?


- Benjamin Hindman


On Aug. 10, 2014, 7:39 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24475/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2014, 7:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Added new DockerInfo for future docker options, and allow command uris to be fetched
and mapped into docker before launching docker container.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto cc9f20e 
>   src/docker/docker.hpp 98b2d60 
>   src/docker/docker.cpp 1cba381 
>   src/slave/containerizer/containerizer.hpp 02754cd 
>   src/slave/containerizer/containerizer.cpp c91ba38 
>   src/slave/containerizer/docker.cpp 904cdd3 
>   src/slave/containerizer/mesos/containerizer.cpp 694c9d1 
>   src/slave/slave.cpp 787bd05 
>   src/tests/docker_containerizer_tests.cpp a559836 
>   src/tests/docker_tests.cpp 4ef1df4 
> 
> Diff: https://reviews.apache.org/r/24475/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message