> On May 21, 2018, 1:21 p.m., Qian Zhang wrote: > > src/slave/containerizer/composing.cpp > > Line 361 (original), 360-365 (patched) > > <https://reviews.apache.org/r/66668/diff/4/?file=2021203#file2021203line361> > > > > So besides removing the container from the `containers_` map, we also > > eliminate a call to `ComposingContainerizerProcess::destroy()`, My > > understanding is we assume the underlying containerizers will always do its > > own destroy to clean up its internal data, so here in composing > > containerizer we just need to remove the container from the `containers_` > > map, right?
>we assume the underlying containerizers will always do its own destroy to >clean up its internal data, so here in composing containerizer we just need to >remove the container from the containers_ map, right? Yes, correct https://github.com/apache/mesos/blob/d7d7cfbc3e5609fc9a4e8de8203a6ecb11afeac7/src/slave/containerizer/containerizer.hpp#L129-L130 > On May 21, 2018, 1:21 p.m., Qian Zhang wrote: > > src/slave/containerizer/composing.cpp > > Lines 396-400 (original) > > <https://reviews.apache.org/r/66668/diff/4/?file=2021203#file2021203line401> > > > > Previously, in the case that `destroy-in-progress stopped an > > launch-in-progress`, the `destroy()` will return `ContainerTermination()` > > which is set here. But now with this implementation, it will return > > `None()` which is set by the underlying containerizer. This seems not > > correct to me, in this case, the destroy should considered as succeeded > > (return `ContainerTermination()`) rather than failed (return `None()`). Before https://reviews.apache.org/r/66510/ patch was committed, `destroy()` returned `Future<bool>`. There was a reason to set a `true` to the `destroyed` promise, because we successfully destroyed a container (in the case that destroy-in-progress stopped an launch-in-progress). >the destroy should considered as succeeded (return ContainerTermination()) >rather than failed (return None()). At the moment I don't think that a client should interpret the result of `destroy()` as failed/succeeded relying on whether or not `Option<ContainerTermination>` is empty. If there was no actual attempt to launch a container using an underlying containerizer, why do we need to return some virtual `ContainerTermination`? - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66668/#review203490 ----------------------------------------------------------- On April 17, 2018, 3:23 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66668/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 3:23 p.m.) > > > Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian > Zhang. > > > Bugs: MESOS-8712 > https://issues.apache.org/jira/browse/MESOS-8712 > > > Repository: mesos > > > Description > ------- > > Previously, we stored `destroyed` promise for each container and used > it to guarantee that `destroy()` returns a non-empty value when the > destroy-in-progress stops an launch-in-progress using the next > containerizer. Since `wait()` and `destroy()` return the same > `ContainerTermination` value when called with the same ContainerID > argument, we can remove `destroyed` promise and add callbacks to > clean up `containers_` map instead. > > > Diffs > ----- > > src/slave/containerizer/composing.cpp > 1fb79f53b48154ecbd3b0165b6a8002c871e6dce > > > Diff: https://reviews.apache.org/r/66668/diff/5/ > > > Testing > ------- > > internal CI > > > Thanks, > > Andrei Budnik > >
