mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 68760: Added a `terminate` function to `ContainerDaemon`.
Date Wed, 19 Sep 2018 15:04:26 GMT

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




src/slave/container_daemon.hpp
Lines 72 (patched)
<https://reviews.apache.org/r/68760/#comment292918>

    Missing comma after `completed`?



src/slave/container_daemon.hpp
Lines 74 (patched)
<https://reviews.apache.org/r/68760/#comment292919>

    `s/he/the/`, `s/reconstruct/construct/`



src/slave/container_daemon.cpp
Lines 86 (patched)
<https://reviews.apache.org/r/68760/#comment292924>

    Nit: Since this does not depend on any parameters from this constructor we could have
set this up in the class declaration,
    ```
    bool terminating = false;
    ```



src/slave/container_daemon.cpp
Lines 99 (patched)
<https://reviews.apache.org/r/68760/#comment292920>

    Toggling `terminated` already here doesn't look right to me as we might fail to `post`
below; I believe we should only set it if we are certain that the agent API has received the
call (is the API idempotent?).



src/slave/container_daemon.cpp
Lines 119 (patched)
<https://reviews.apache.org/r/68760/#comment292921>

    We currently don't need the `defer` here, but will need to once we set `terminated` here,
see above.



src/slave/container_daemon.cpp
Lines 130-131 (patched)
<https://reviews.apache.org/r/68760/#comment292922>

    The usual pattern elsewhere seems to be to use an `onAny` instead of a `then` above, and
then check in the continutation whether `response.isReady` and conditionally return early
there.



src/slave/container_daemon_process.hpp
Lines 57 (patched)
<https://reviews.apache.org/r/68760/#comment292923>

    Nit: Should this be named `terminateContainer`/`stopContainer` for symmetry with `launchContainer`?
Maybe not as we likely would want this method to always remain `public`.


- Benjamin Bannier


On Sept. 19, 2018, 6:52 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68760/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2018, 6:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9228
>     https://issues.apache.org/jira/browse/MESOS-9228
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `terminate` function will send a kill to its monitoring container
> and cause the monitor loop to stop. The loop will be stopped after
> the `postStopHook` is called.
> 
> 
> Diffs
> -----
> 
>   src/slave/container_daemon.hpp a58140dd787524b0438ba4719fa7e3365ab2fa17 
>   src/slave/container_daemon.cpp e1b0812b8b467ee4061b23d39552e2417d65a14a 
>   src/slave/container_daemon_process.hpp a5d19a04223f6f595e97ec83962558639fd51f7b 
> 
> 
> Diff: https://reviews.apache.org/r/68760/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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