mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Budnik <abud...@mesosphere.com>
Subject Re: Review Request 66668: Removed `destroyed` from `Container` struct in composing containerizer.
Date Fri, 11 May 2018 13:04:31 GMT


> On May 9, 2018, 3:15 p.m., Qian Zhang wrote:
> > I checked the callers of `Containerizer::destroy()`, it seems no one actually cares
about its return value (`Option<ContainerTermination>`), so why do we need to return
that? Can we just make it return `Future<Nothing>`?
> 
> Qian Zhang wrote:
>     And then for the last line of `ComposingContainerizerProcess::destroy`:
>     ```
>     return container->containerizer->wait(containerId);
>     ```
>     We could change it to something like:
>     ```
>     return container->containerizer->wait(containerId)
>         .onAny([](const Future<Option<ContainerTermination>>& termination)
{return Nothing();})
>     ```

There are plans to use the return value of `destroy()` in tests, see [MESOS-8829](https://issues.apache.org/jira/browse/MESOS-8829).

Also, as `destroy()` depends on `wait()` method, it'd be necessary to implement wrappers like
your example above, if we want them to return different type. Those wrappers introduce some
extra complexity, which IMO doesn't facilitate achieving one of the goals "to simplify composing
containerizer".


> On May 9, 2018, 3:15 p.m., Qian Zhang wrote:
> > src/slave/containerizer/composing.cpp
> > Lines 658-661 (original)
> > <https://reviews.apache.org/r/66668/diff/2/?file=2012420#file2012420line658>
> >
> >     If we remove these codes, will the container be missed to delete and erased
from `containers_`?
> 
> Qian Zhang wrote:
>     Ah, Just saw the code to delete it in the next patch. Anyway I think it may be better
to put that change into this patch :-)

`container_` cleanup is implemented in the following patch `/r/66669/`.


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66668/#review202746
-----------------------------------------------------------


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 were using `destroyed` promise for each container to
> guarantee that calling `destroy()` on a container returns a non-empty
> value when the destroy-in-progress stops an launch-in-progress using
> the next containerizer. Since we are unifying the semantics of the
> return type for both `wait()` and `destroy()` operations, we can
> remove the `destroyed` promise.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/composing.cpp 186102c66d373dcd799cadd9fed7d1c8cb894971 
> 
> 
> Diff: https://reviews.apache.org/r/66668/diff/2/
> 
> 
> Testing
> -------
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>


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