mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.
Date Mon, 21 May 2018 20:05:50 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message