mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 63680: Added a container daemon to monitor a long-running standalone container.
Date Wed, 15 Nov 2017 22:26:11 GMT

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




src/slave/container_daemon.hpp
Lines 40 (patched)
<https://reviews.apache.org/r/63680/#comment268692>

    We typically don't use javadoc-style comments for internal classes.



src/slave/container_daemon.hpp
Lines 88-90 (patched)
<https://reviews.apache.org/r/63680/#comment268698>

    Looks like the intent of this method is to return when a loop-breaking error occurs. 
You should probably mention this.
    
    And that the only way to retry (given the current interface) is to reconstruct the ContainerDaemon
object.



src/slave/container_daemon.hpp
Lines 89 (patched)
<https://reviews.apache.org/r/63680/#comment268694>

    s/discraded/discarded/



src/slave/container_daemon.cpp
Lines 136-147 (patched)
<https://reviews.apache.org/r/63680/#comment268693>

    Style nit: Single newline needed before each `if` statement.
    
    Also, try spacing the protobuf mutation like:
    ```
    launchCall.mutable_launch_container()
      ->mutable_command()->CopyFrom(commandInfo.get());
    ```



src/slave/container_daemon.cpp
Lines 170 (patched)
<https://reviews.apache.org/r/63680/#comment268695>

    s/contanier/container/



src/slave/container_daemon.cpp
Lines 188-198 (patched)
<https://reviews.apache.org/r/63680/#comment268704>

    Unless you have plans to add more than one call site for this helper method, you should
just inline the contents into `launchContainer`.  Same thing with `stopped` and `waitContainer`.



src/slave/container_daemon.cpp
Lines 216-219 (patched)
<https://reviews.apache.org/r/63680/#comment268702>

    It doesn't seem necessary to have a mutex here, especially since all the calls are initiated
from within this process.  There are no other actors that can Launch or Kill.
    
    There's only one place where the Kill is used anyway.  So there's no way for the mutex
to come into play.  When the `force` parameter is true, the control flow is basically linear:
    
    kill -> wait -> 
      launch -> wait -> 
      ...


- Joseph Wu


On Nov. 15, 2017, 1:21 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63680/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2017, 1:21 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8183
>     https://issues.apache.org/jira/browse/MESOS-8183
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `ContanierDaemon` class is responsible to monitor if a long-running
> service running in a standalone container, and restart the service
> container after it exits. It takes the following hook functions:
> 
> * `postStartHook`: called after the container is launched every time.
> * `postStopHook`: called after the container exits every time.
> 
> `ContainerDaemon` does not manage the lifecycle of the contanier it
> monitors, so the container persists across `ContainerDaemon`s. However,
> it provides a `terminate()` method to completely shutdown the service
> container.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 955f01a665019d7750980ff16f126ad63b524594 
>   src/slave/container_daemon.hpp PRE-CREATION 
>   src/slave/container_daemon.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63680/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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