You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2018/04/09 15:44:57 UTC
Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
Bugs: MESOS-8706
https://issues.apache.org/jira/browse/MESOS-8706
Repository: mesos
Description
-------
This patch changes the return type of `destroy()` method for all
containerizers. As a result, `destroy()` and `wait()` methods have
got the same signature and return type. In fact, all these methods
depend on the same container termination promise, so this unification
makes both containerizer interface and internal logic more consistent.
Diffs
-----
src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
Diff: https://reviews.apache.org/r/66510/diff/1/
Testing
-------
Tests can't be compiled after this change.
See the next patch in the chain.
Thanks,
Andrei Budnik
Re: Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 10, 2018, 6:15 p.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 2335 (original), 2336 (patched)
> > <https://reviews.apache.org/r/66510/diff/1/?file=1994282#file1994282line2336>
> >
> > Hmm I think it might be a bit more readable here and elsewhere to continue using a trivial lambda which returns a default-constructed `ContainerTermination`, WDYT?
> >
> > ```
> > .then([]() { return ContainerTermination(); });
> > ```
>
> Andrei Budnik wrote:
> `Option<ContainerTermination>::some` is a wrapper for `ContainerTermination` type: https://github.com/apache/mesos/blob/3dcd225dbb817da676ae57f4f8871650ece5a0b8/3rdparty/stout/include/stout/option.hpp#L36-L39
> So, this code returns a wrapped termination variable instead of constructing a new one.
>
> BTW, the shortest equivalent code would be:
> ```
> .then([](const Future<ContainerTermination>& termination) -> Option<ContainerTermination> {
> return termination.get();
> });
> ```
> which doesn't look simpler to me.
Ah OK sorry, makes sense.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/#review200828
-----------------------------------------------------------
On April 9, 2018, 3:44 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 3:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
>
>
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
> src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
> src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
> src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
> src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
> src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
> src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
> src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
>
>
> Diff: https://reviews.apache.org/r/66510/diff/2/
>
>
> Testing
> -------
>
> Tests can't be compiled after this change.
> See the next patch in the chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On April 10, 2018, 6:15 p.m., Greg Mann wrote:
> > src/slave/containerizer/composing.cpp
> > Line 376 (original), 378 (patched)
> > <https://reviews.apache.org/r/66510/diff/1/?file=1994277#file1994277line378>
> >
> > Is it not possible to use `None()` here instead of `Option<ContainerTermination>::none()`? Here and elsewhere.
I'm getting compilation error with `None()`. Moreover, I can't find `set(None())` pattern, but I found one place where we use `set(Option<int>::none())`: https://github.com/apache/mesos/blob/3dcd225dbb817da676ae57f4f8871650ece5a0b8/3rdparty/libprocess/src/reap.cpp#L122
> On April 10, 2018, 6:15 p.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 2335 (original), 2336 (patched)
> > <https://reviews.apache.org/r/66510/diff/1/?file=1994282#file1994282line2336>
> >
> > Hmm I think it might be a bit more readable here and elsewhere to continue using a trivial lambda which returns a default-constructed `ContainerTermination`, WDYT?
> >
> > ```
> > .then([]() { return ContainerTermination(); });
> > ```
`Option<ContainerTermination>::some` is a wrapper for `ContainerTermination` type: https://github.com/apache/mesos/blob/3dcd225dbb817da676ae57f4f8871650ece5a0b8/3rdparty/stout/include/stout/option.hpp#L36-L39
So, this code returns a wrapped termination variable instead of constructing a new one.
BTW, the shortest equivalent code would be:
```
.then([](const Future<ContainerTermination>& termination) -> Option<ContainerTermination> {
return termination.get();
});
```
which doesn't look simpler to me.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/#review200828
-----------------------------------------------------------
On April 9, 2018, 3:44 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 3:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
>
>
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
> src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
> src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
> src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
> src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
> src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
> src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
> src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
>
>
> Diff: https://reviews.apache.org/r/66510/diff/1/
>
>
> Testing
> -------
>
> Tests can't be compiled after this change.
> See the next patch in the chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/#review200828
-----------------------------------------------------------
src/slave/containerizer/composing.cpp
Line 376 (original), 378 (patched)
<https://reviews.apache.org/r/66510/#comment281728>
Is it not possible to use `None()` here instead of `Option<ContainerTermination>::none()`? Here and elsewhere.
src/slave/containerizer/mesos/containerizer.cpp
Line 2335 (original), 2336 (patched)
<https://reviews.apache.org/r/66510/#comment281732>
Hmm I think it might be a bit more readable here and elsewhere to continue using a trivial lambda which returns a default-constructed `ContainerTermination`, WDYT?
```
.then([]() { return ContainerTermination(); });
```
- Greg Mann
On April 9, 2018, 3:44 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 3:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
>
>
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
> src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
> src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
> src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
> src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
> src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
> src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
> src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
>
>
> Diff: https://reviews.apache.org/r/66510/diff/1/
>
>
> Testing
> -------
>
> Tests can't be compiled after this change.
> See the next patch in the chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
Posted by Qian Zhang <zh...@gmail.com>.
> On May 9, 2018, 4:34 p.m., Qian Zhang wrote:
> > src/slave/containerizer/composing.cpp
> > Line 375 (original), 377 (patched)
> > <https://reviews.apache.org/r/66510/diff/2/?file=1995837#file1995837line377>
> >
> > Not yours, but `after launch returned false` seems not correct to me since launch does not return true/false anymore. So it should be something like `after launch returned NOT_SUPPORTED`?
I see such kind of comments a couple of times in composing.cpp, better to update all of them in a separte patch.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/#review202736
-----------------------------------------------------------
On April 9, 2018, 11:44 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 11:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
>
>
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
> src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
> src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
> src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
> src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
> src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
> src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
> src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
>
>
> Diff: https://reviews.apache.org/r/66510/diff/2/
>
>
> Testing
> -------
>
> Tests can't be compiled after this change.
> See the next patch in the chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/#review202736
-----------------------------------------------------------
src/slave/containerizer/composing.cpp
Line 375 (original), 377 (patched)
<https://reviews.apache.org/r/66510/#comment284717>
Not yours, but `after launch returned false` seems not correct to me since launch does not return true/false anymore. So it should be something like `after launch returned NOT_SUPPORTED`?
src/slave/containerizer/composing.cpp
Line 516 (original), 519 (patched)
<https://reviews.apache.org/r/66510/#comment284718>
Ditto.
- Qian Zhang
On April 9, 2018, 11:44 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 11:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
>
>
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
> src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
> src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
> src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
> src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
> src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
> src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
> src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
>
>
> Diff: https://reviews.apache.org/r/66510/diff/2/
>
>
> Testing
> -------
>
> Tests can't be compiled after this change.
> See the next patch in the chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 66510: Unified return type of `wait` and `destroy`
containerizer methods.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66510/#review200937
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On April 9, 2018, 3:44 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 3:44 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian Zhang.
>
>
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.hpp 1221d9b61b7cfbdbaa560dbafb0158225331ed6d
> src/slave/containerizer/composing.cpp cd840a5409023d32701329bf37806f2e7ed6ffd2
> src/slave/containerizer/containerizer.hpp 836283aa8aa827459558d567e090a230c32351ed
> src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635
> src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f
> src/slave/containerizer/mesos/containerizer.hpp cba4ed2e20622070b46071f35907d8b32c25b2fc
> src/slave/containerizer/mesos/containerizer.cpp 7473a871e626833733f39375b778aff70529dc63
> src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd
>
>
> Diff: https://reviews.apache.org/r/66510/diff/2/
>
>
> Testing
> -------
>
> Tests can't be compiled after this change.
> See the next patch in the chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>