> On Sept. 19, 2018, 3:04 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon.hpp > > Lines 72 (patched) > > <https://reviews.apache.org/r/68760/diff/1/?file=2090236#file2090236line72> > > > > Missing comma after `completed`?
No there's no missing comma. Let me rephrase it to `The looper is stopped after the `postStopHook` function is completed if `terminate` has been called, or ...`. > On Sept. 19, 2018, 3:04 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon.cpp > > Lines 99 (patched) > > <https://reviews.apache.org/r/68760/diff/1/?file=2090237#file2090237line99> > > > > 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?). The `terminating` is also a control for `launchContainer` to break the loop before launching the next container, so it seems more correct to me to set the flag here. This is also why I added a TODO for retry. I can go implementing the retry if you are concerned loosing connections between two actors in the same agent linux process. The invariant I'm trying to maintain here is that once `terminated` is completed, no other dispatches will go to the actor. It seems I've made a mistake below ;) > On Sept. 19, 2018, 3:04 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon.cpp > > Lines 119 (patched) > > <https://reviews.apache.org/r/68760/diff/1/?file=2090237#file2090237line119> > > > > We currently don't need the `defer` here, but will need to once we set > > `terminated` here, see above. We need to defer here because the lambda uses `this->containerId`. But actually we should not defer here to make sure that no dispatch will go to the actor once `terminated` is completed. > On Sept. 19, 2018, 3:04 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon.cpp > > Lines 130-131 (patched) > > <https://reviews.apache.org/r/68760/diff/1/?file=2090237#file2090237line130> > > > > 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. It seems we use both patterns in different places and I feel that this pattern reads slightly better to me, as we don't have the conditional return. > On Sept. 19, 2018, 3:04 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon_process.hpp > > Lines 57 (patched) > > <https://reviews.apache.org/r/68760/diff/1/?file=2090238#file2090238line57> > > > > Nit: Should this be named `terminateContainer`/`stopContainer` for > > symmetry with `launchContainer`? Maybe not as we likely would want this > > method to always remain `public`. I'm following this pattern: 1. Spawn the actor in the constructor. 2. Provide a `void terminate()` for the callor to stop the component. 3. Provide a `Future<Nothing> wait()` for the caller to asnychronously waits for the termination (not necessarily triggered by 2). Alternatively, we can have the following pattern instead (which is used by the libprocess http server): 1. Provide a `Future<Nothing> run()` to spawn the actor and return a future which will be completed upon the termination of the component. 2. PRovide a `Future<Nothing> stop()` for the caller to stop the component and wait for its termination. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68760/#review208765 ----------------------------------------------------------- On Sept. 19, 2018, 4: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, 4: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 > >
