You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2016/04/03 15:06:18 UTC
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/
-----------------------------------------------------------
(Updated April 3, 2016, 1:06 p.m.)
Review request for mesos and Till Toenshoff.
Changes
-------
Rebase.
Repository: mesos
Description
-------
Remove `SlaveState` in `DockerContainerizer` during recover.
Diffs (updated)
-----
src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
Diff: https://reviews.apache.org/r/44823/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
Posted by haosdent huang <ha...@gmail.com>.
> On April 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 794-797
> > <https://reviews.apache.org/r/44823/diff/4/?file=1323359#file1323359line794>
> >
> > const ExecutorInfo& executorInfo = recoverable.executor_info();
> > const ContainerID& containerId = recoverable.container_id();
> > const FrameworkID& frameworkId = executorInfo.framework_id();
> > const ExecutorID& executorId = executorInfo.executor_id();
Ditto here, see https://issues.apache.org/jira/browse/MESOS-2629
> On April 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 825-826
> > <https://reviews.apache.org/r/44823/diff/4/?file=1323359#file1323359line825>
> >
> > I think that here should be `||` but not `&&`
Actually these code existed before. If change to `||`, those containers could be found in `existingContainers` and `executorInfo.has_container()` is false would not be recovered.
> On April 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 825-839
> > <https://reviews.apache.org/r/44823/diff/4/?file=1323359#file1323359line825>
> >
> > I think that this logic can make the code more clear:
> >
> > if (!existingContainers.contains(containerId)) {
> > xxx;
> > continue;
> > }
> >
> > if (!executorInfo.has_container()) {
> > xxx;
> > continue;
> > }
> >
> > if (executorInfo.container().type() != ContainerInfo::DOCKER) {
> > xxxx;
> > continue;
> > }
Actually we could not skip recover when `!existingContainers.contains(containerId) && executorInfo.has_container()`. Below code would skip this case.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/#review126728
-----------------------------------------------------------
On April 3, 2016, 2:11 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44823/
> -----------------------------------------------------------
>
> (Updated April 3, 2016, 2:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, and Timothy Chen.
>
>
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove `SlaveState` in `DockerContainerizer` during recover.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
> src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
> src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
>
> Diff: https://reviews.apache.org/r/44823/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 825-826
> > <https://reviews.apache.org/r/44823/diff/4/?file=1323359#file1323359line825>
> >
> > I think that here should be `||` but not `&&`
>
> haosdent huang wrote:
> Actually these code existed before. If change to `||`, those containers could be found in `existingContainers` and `executorInfo.has_container()` is false would not be recovered.
I think that when recover, we should fileter out the continers whose executor does not include container info or the does not recognized by the `existingContainers`?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/#review126728
-----------------------------------------------------------
On 四月 3, 2016, 2:11 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44823/
> -----------------------------------------------------------
>
> (Updated 四月 3, 2016, 2:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, and Timothy Chen.
>
>
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove `SlaveState` in `DockerContainerizer` during recover.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
> src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
> src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
>
> Diff: https://reviews.apache.org/r/44823/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
Posted by haosdent huang <ha...@gmail.com>.
> On April 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 825-826
> > <https://reviews.apache.org/r/44823/diff/4/?file=1323359#file1323359line825>
> >
> > I think that here should be `||` but not `&&`
>
> haosdent huang wrote:
> Actually these code existed before. If change to `||`, those containers could be found in `existingContainers` and `executorInfo.has_container()` is false would not be recovered.
>
> Guangya Liu wrote:
> I think that when recover, we should fileter out the continers whose executor does not include container info or the does not recognized by the `existingContainers`?
If we filter out all executorInfo that doesn't contain `container()`, we would miss this case: the containerId exists in existingContainers and its `executorInfo` doesn't have container().
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/#review126728
-----------------------------------------------------------
On April 4, 2016, 4:48 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44823/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 4:48 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, and Timothy Chen.
>
>
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove `SlaveState` in `DockerContainerizer` during recover.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
> src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
> src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
>
> Diff: https://reviews.apache.org/r/44823/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/#review126728
-----------------------------------------------------------
src/slave/containerizer/docker.cpp (lines 785 - 788)
<https://reviews.apache.org/r/44823/#comment189785>
const ExecutorInfo& executorInfo = recoverable.executor_info();
const ContainerID& containerId = recoverable.container_id();
const FrameworkID& frameworkId = executorInfo.framework_id();
const ExecutorID& executorId = executorInfo.executor_id();
src/slave/containerizer/docker.cpp (lines 790 - 791)
<https://reviews.apache.org/r/44823/#comment189786>
I think that here should be `||` but not `&&`
src/slave/containerizer/docker.cpp (lines 790 - 803)
<https://reviews.apache.org/r/44823/#comment189787>
I think that this logic can make the code more clear:
if (!existingContainers.contains(containerId)) {
xxx;
continue;
}
if (!executorInfo.has_container()) {
xxx;
continue;
}
if (executorInfo.container().type() != ContainerInfo::DOCKER) {
xxxx;
continue;
}
- Guangya Liu
On 四月 3, 2016, 2:11 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44823/
> -----------------------------------------------------------
>
> (Updated 四月 3, 2016, 2:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, and Timothy Chen.
>
>
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Remove `SlaveState` in `DockerContainerizer` during recover.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
> src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
> src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
>
> Diff: https://reviews.apache.org/r/44823/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/
-----------------------------------------------------------
(Updated April 4, 2016, 4:48 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, and Timothy Chen.
Changes
-------
Address @gyliu's comments.
Bugs: MESOS-5085
https://issues.apache.org/jira/browse/MESOS-5085
Repository: mesos
Description
-------
Remove `SlaveState` in `DockerContainerizer` during recover.
Diffs (updated)
-----
src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
Diff: https://reviews.apache.org/r/44823/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer`
during recover.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44823/
-----------------------------------------------------------
(Updated April 3, 2016, 2:11 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, and Timothy Chen.
Bugs: MESOS-5085
https://issues.apache.org/jira/browse/MESOS-5085
Repository: mesos
Description
-------
Remove `SlaveState` in `DockerContainerizer` during recover.
Diffs
-----
src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda
src/slave/containerizer/docker.cpp 9314d1f9e0b6077fe7c48b860783ab21acc48be6
src/tests/containerizer/docker_containerizer_tests.cpp b9c26b92c44e8ca718a5eb333855199bdf2a8e81
Diff: https://reviews.apache.org/r/44823/diff/
Testing
-------
Thanks,
haosdent huang