> On May 18, 2018, 5:26 p.m., Greg Mann wrote: > > src/slave/containerizer/composing.cpp > > Line 669 (original), 618 (patched) > > <https://reviews.apache.org/r/66668/diff/4/?file=2021203#file2021203line679> > > > > Why return `wait()` when the state is DESTROYING, rather than just > > forwarding the call to the underlying containerizer's `destroy()`? > > > > With this implementation, the composing containerizer's `wait()` will > > behave differently than the underlying containerizer's `wait()`, is that > > what we want? > > Andrei Budnik wrote: > Good point! > It looks like `container->containerizer->destroy(containerId).onAny(...)` > can be moved from `if` condition to the bottom of the method. That will > simplify `ComposingContainerizerProcess::destroy()` implementation. > WDYT? > > Qian Zhang wrote: > > Why return wait() when the state is DESTROYING, rather than just > forwarding the call to the underlying containerizer's destroy()? > > `DESTROYING` state means a call to the underlying containerizer's > `destroy()` is already going on, so why do we want to call it again in this > case? > > > With this implementation, the composing containerizer's wait() will > behave differently than the underlying containerizer's wait(), is that what > we want? > > How will the composing containerizer's `wait()` behave differently than > the underlying containerizer's `wait()`? I see it is actually just a wrapper > of the underlying containerizer's `wait()`.
> DESTROYING state means a call to the underlying containerizer's destroy() is > already going on, so why do we want to call it again in this case? My logic was that the behavior of the composing containerizer should be the same as the individual containerizers. So, if the caller calls `destroy()` a second time, we can just forward the call to the individual containerizer. > How will the composing containerizer's wait() behave differently than the > underlying containerizer's wait()? I see it is actually just a wrapper of the > underlying containerizer's wait(). Sorry, what I meant to type was: With this implementation, the composing containerizer's `destroy()` will behave differently than the underlying containerizer's `destroy()`, is that what we want? - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66668/#review203428 ----------------------------------------------------------- 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/4/ > > > Testing > ------- > > internal CI > > > Thanks, > > Andrei Budnik > >
