You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/05/02 02:04:36 UTC
Review Request 58903: Combined Mesos containerizer's launch methods.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58903/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Bugs: MESOS-7449
https://issues.apache.org/jira/browse/MESOS-7449
Repository: mesos
Description
-------
This simplifies the container launch path by removing combining
the nested and non-nested container code paths into one.
The Mesos containerizer was originally translating the two
`containerizer->launch` entrypoints into a common method (also
called `launch`). The previous commits moved this translation
logic into the caller (i.e. the Agent).
The end result has some slight changes:
* It is now possible for the Agent to specify some more combinations
of `ContainerConfig`. For example, specifying a TaskInfo with
a DEBUG-class container. Or a nested container with Resources.
We may need to add extra validation around this.
* The `bool checkpoint` argument was replaced with a `Option<string>`
which optionally contains an absolute path. This allows us to
remove the `SlaveID` field.
Diffs
-----
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
Diff: https://reviews.apache.org/r/58903/diff/1/
Testing
-------
See last patch in chain.
Thanks,
Joseph Wu
Re: Review Request 58903: Combined Mesos containerizer's launch
methods.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58903/#review174637
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/containerizer.cpp
Lines 984-986 (original), 931-933 (patched)
<https://reviews.apache.org/r/58903/#comment247834>
In fact, I would call the parameter `_containerConfig` here:
```
ContainerConfig containerConfig = _containerConfig;
```
And moving forward, consistently use `containerConfig` in this function.
src/slave/containerizer/mesos/containerizer.cpp
Line 1066 (original), 1037 (patched)
<https://reviews.apache.org/r/58903/#comment247832>
It's weird that you still `containerConfig` not `containerConfig_` here.
See my comments above.
- Jie Yu
On May 2, 2017, 2:04 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58903/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 2:04 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7449
> https://issues.apache.org/jira/browse/MESOS-7449
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This simplifies the container launch path by removing combining
> the nested and non-nested container code paths into one.
>
> The Mesos containerizer was originally translating the two
> `containerizer->launch` entrypoints into a common method (also
> called `launch`). The previous commits moved this translation
> logic into the caller (i.e. the Agent).
>
> The end result has some slight changes:
> * It is now possible for the Agent to specify some more combinations
> of `ContainerConfig`. For example, specifying a TaskInfo with
> a DEBUG-class container. Or a nested container with Resources.
> We may need to add extra validation around this.
> * The `bool checkpoint` argument was replaced with a `Option<string>`
> which optionally contains an absolute path. This allows us to
> remove the `SlaveID` field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
>
>
> Diff: https://reviews.apache.org/r/58903/diff/1/
>
>
> Testing
> -------
>
> See last patch in chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 58903: Combined Mesos containerizer's launch
methods.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58903/
-----------------------------------------------------------
(Updated May 19, 2017, 3:10 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
Addressed comments.
Bugs: MESOS-7449
https://issues.apache.org/jira/browse/MESOS-7449
Repository: mesos
Description
-------
This simplifies the container launch path by removing combining
the nested and non-nested container code paths into one.
The Mesos containerizer was originally translating the two
`containerizer->launch` entrypoints into a common method (also
called `launch`). The previous commits moved this translation
logic into the caller (i.e. the Agent).
The end result has some slight changes:
* It is now possible for the Agent to specify some more combinations
of `ContainerConfig`. For example, specifying a TaskInfo with
a DEBUG-class container. Or a nested container with Resources.
We may need to add extra validation around this.
* The `bool checkpoint` argument was replaced with a `Option<string>`
which optionally contains an absolute path. This allows us to
remove the `SlaveID` field.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
src/slave/containerizer/mesos/containerizer.cpp 50a63b58b4960729316b0b5685793ce18ee5ce93
Diff: https://reviews.apache.org/r/58903/diff/2/
Changes: https://reviews.apache.org/r/58903/diff/1-2/
Testing
-------
See last patch in chain.
Thanks,
Joseph Wu