> On Sept. 19, 2018, 1:38 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon.hpp > > Lines 86 (patched) > > <https://reviews.apache.org/r/68759/diff/1/?file=2090233#file2090233line87> > > > > Any reason we don't just use a `process::Promise` terminated here > > instead of sharing state?
The future comes from a promise inside `ContainerDaemonProcess`, who is responsible to set the promise. Not sure what you mean by "sharing state." > On Sept. 19, 2018, 1:38 p.m., Benjamin Bannier wrote: > > src/slave/container_daemon.cpp > > Line 262 (original), 263 (patched) > > <https://reviews.apache.org/r/68759/diff/1/?file=2090234#file2090234line294> > > > > That we don't `wait` for the wrapped process to terminate here seems > > significant. Could you explain why that is okay? We had some offline discussions. The major benefit of getting rid of `process::wait` is because we'll destruct `ContainerDaemon`s in a worker thread, and `process::wait` is a blocking call which should be avoided in any worker thread. BenM also mentioned that the long-term goal is to make all actors auto-managed and get rid of all `process::wait` calls in destructors (or the alternative is to allow `process::wait` but move the thread out from the worker thread pool). In general, there is no restriction to create two `ContainerDaemon`s to monitor the same container, and doing a `process::wait` here won't prevent this from happening. However in the original code, because we only create one single SLRP for a (type, name) pair at a time, the `process::wait` here essentially creates a synchronization between two SLRP instances of the same (type, name) pair to guarantee that there is at most one `ContainerDaemonProcess` to monitor the container at a time. To achieve the same, I'm proposing to remove `process::wait` here, but instead asking the caller to wait for `ContainerDaemon::wait` as the synchronization between two SLRP instances. I could keep the `process::wait` here for now if you think it's better. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68759/#review208763 ----------------------------------------------------------- On Sept. 19, 2018, 4:51 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68759/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2018, 4:51 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 > ------- > > This patch made the following changes to the container daemon: > 1. Made it stores the launch parameters instead of an `agent::Call`. > 2. Cleaned up the log messages. > 3. Made the process managed by libprocess. > > > 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/68759/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
