mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 29889: Recover Docker containers when mesos slave is in a container
Date Tue, 12 May 2015 01:58:06 GMT

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



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

    1. There is no declaration in the hpp file with a comment and there is no explanation
here. I would suggest using "static" here so nobody needs to browse the hpp file in search
for a potential comment that does not exist.
    
    2. OK, I can guess what this does, but future readers may want to skip reading the function
body to find out. The name suggests running something.
    
    3. I can't find where this function is used.
    
    It seems to me that you wanted to break out part of Docker::run() below, but have not
removed this code there and replaced it with a call yet.



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

    This is a bit tricky. You have to know that 'stop' receives a non-Nothing value once 'run'
gets one. Suggestion: reduce this to only one future - 'run'. See below in line 170.



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

    Why can't we call 'stop.onAny(...)' directly here?



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

    s/might still/might still be



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

    Suggestions: 
    
    Break this out as a separate statement/comment pair.
    
    s/copy to remove const/mutable copy of the future



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

    Could we use a LOCAL future 'stop' instead and discard 'run' once it is satisfied? This
way 'stop' does not have to be a global and one can see here locally what is going on.
    
    You could then amend _reaped() for checking for run having been discarded.



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

    See line 136, it seems that we do not need this function.
    
    If you were to keep it, it would be better placed above _reaped, so that one could read
"upper half / lower half" contiguously.



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

    Aren't we going to inspect 'stop'? What if it failed? Could we at least log something
then?



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

    s/after stopping/after attempting to stop



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

    No default? Why?



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

    Which docker image?



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

    "only"? What other cases are there?



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

    Which of these arguments signifies the executor? 
    'container'? Could we rename it accordingly?
    
    Before you updated the comment, I overlooked that this might be executor-specific code!



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment134310>

    We should generally avoid bool args, because it is hard to see at the call site what they
mean.
    
    Suggestions:
    - Use an enum.
    - Keep the exists() and running() methods, factor out what's common between them, pass
a closure as an arg to the subroutine for what's different.



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment134311>

    Insert blank line above, please.
    
    In other places below, too.
    
    Maybe better: move the comment above the declaration.



src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment134314>

    What changed so that we don't need this test any more?


- Bernd Mathiske


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


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