> 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()`). > > Andrei Budnik wrote: > 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`? > > Greg Mann wrote: > Hm, so we don't have any callers who need to recognize the > destroy-during-launch case? If not, then I think this sounds OK to me, WDYT > Qian? Andrei, if we decide to keep this approach, could you update the > comment on `destroy()` in the following patch to better reflect what happens > when it's called during container launch? https://reviews.apache.org/r/67130/ > > Qian Zhang wrote: > OK to me as well.
Updated comments: https://reviews.apache.org/r/66668/diff/6-7/ https://reviews.apache.org/r/67130/diff/3-4/ - 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/7/ > > > Testing > ------- > > internal CI > > > Thanks, > > Andrei Budnik > >
