mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 29334: Add option to launch docker containers with helper containers.
Date Sun, 18 Jan 2015 12:18:05 GMT

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



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

    At this compleity level, the comments here have begun to look like an anti-pattern that
might create unwanted precedent for others to mimic. IMHO this was already the case before
this patch, but the current additions exacerbate it.
    
    The naming launchInContainer helps, but it is still in line with the expected style? 
    
    It seems to me that the expected style breaks down here. Ideas?



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

    IMHO readability is subverted by prolonging the underscore scheme when there is no strict
series of continuations. This code would be so much more easy to read if there were descriptive
method names. Absent these, please add comments that summarize the purpose and general approach
of these methods in places such as this one.



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

    Here one can see that we are dealing with a more general graph than a series. How can
one know why ___launch (3 underscores) is not needed in this branch?
    
    And how can one know what ___launch, ____launch, _____launch do in the first place? One
must read them all, memorize them, and then read this stretch of code again. Ideas?



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

    When using names such as "___launch" without any trace of what the continuation specified
actually does, then we need a comment somewhere to explain purpose and general approach. This
can be minimal, but putting nothing at all forces inefficient depth-first code reading.



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

    I would group this differently:
    
    ContainerInfo::DockerInfo dockerInfo;
    dockerInfo.set_image(flags.docker_mesos_image.get());
    
    ContainerInfo containerInfo;
    containerInfo.mutable_docker()->CopyFrom(dockerInfo);



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

    "mesos-docker-executor" comes out of nowhere. Declaring it as a constant (possibly, but
not necessarily in this file) would provide a natural way to link to background information
(e.g. what exactly this program does and where to find it). This info could then be found
trivially by looking up said declaration, where one would place some explanations.



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

    It would be clearer not reusing the volume variable here.



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

    Not checking the outcome of this?



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

    not -> not have
    on -> in
    has -> had been
    executor -> executors


- Bernd Mathiske


On Jan. 16, 2015, 5:36 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29334/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2015, 5:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Add option to launch docker containers with helper containers.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/docker.hpp b7bf54a 
>   src/slave/containerizer/docker.cpp 5f4b4ce 
> 
> Diff: https://reviews.apache.org/r/29334/diff/
> 
> 
> Testing
> -------
> 
> make, tests are fixed in next commit
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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