Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer` during recover.

[haosdent huang]

> On April 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 825-826
> > 
> >
> > 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.

[haosdent huang]

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

[Guangya Liu]

> On 四月 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 825-826
> > 
> >
> > 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.

[haosdent huang]

> On April 3, 2016, 2:22 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 794-797
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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) {
> >   ;
> >   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.

[Guangya Liu]

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


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)


I think that here should be `||` but not `&&`



src/slave/containerizer/docker.cpp (lines 790 - 803)


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) {
  ;
  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.

[haosdent huang]

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

[haosdent huang]

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

[haosdent huang]

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

(Updated March 16, 2016, 11:26 a.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 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
  src/tests/containerizer/docker_containerizer_tests.cpp 
8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 

Diff: https://reviews.apache.org/r/44823/diff/


Testing
---


Thanks,

haosdent huang

Re: Review Request 44823: Remove `SlaveState` in `DockerContainerizer` during recover.

[haosdent huang]

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

(Updated March 16, 2016, 5:56 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 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f6fce7df82417e029fadf805d6e0b793f396aa69 

Diff: https://reviews.apache.org/r/44823/diff/


Testing
---


Thanks,

haosdent huang

Review Request 44823: Remove `SlaveState` in `DockerContainerizer` during recover.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `SlaveState` in `DockerContainerizer` during recover.


Diffs
-

  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
  src/tests/containerizer/docker_containerizer_tests.cpp 
8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 

Diff: https://reviews.apache.org/r/44823/diff/


Testing
---


Thanks,

haosdent huang