You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/12/23 00:52:38 UTC

Review Request 29336: Recover docker containers that launched in containers.

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

Review request for mesos and Benjamin Hindman.


Repository: mesos-git


Description
-------

Recover docker containers that launched in containers.


Diffs
-----

  src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815 
  src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665 

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


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Timothy Chen <tn...@apache.org>.

> On Jan. 12, 2015, 5:30 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.cpp, line 112
> > <https://reviews.apache.org/r/29336/diff/2/?file=813856#file813856line112>
> >
> >     According to review #29341 this is wrong. See your updated solution there! The latter seems to be the correct one to me.

Yes I fix this in a later review. I uploaded the whole commit chain, where the latest has some fixes.


- Timothy


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


On Jan. 9, 2015, 1:25 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29336/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2015, 1:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Recover docker containers that launched in containers.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/docker.hpp b7bf54a 
>   src/slave/containerizer/docker.cpp 5f4b4ce 
> 
> Diff: https://reviews.apache.org/r/29336/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29336/#review67660
-----------------------------------------------------------


This code was relatively hard to get into. Please be generous with strategy/overview comments!


src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111722>

    According to review #29341 this is wrong. See your updated solution there! The latter seems to be the correct one to me.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111723>

    This method is now officially too large.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111724>

    This could be broken out and given a nice method name.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111725>

    Another section that could be in its own method starts here.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111726>

    executorPid? waitPid? lastExecutorRunPid? containerPid? I am guessing the former.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111731>

    This can be broken out, too.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111732>

    that pid of the -> the pid of whose



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment111734>

    a -> an


- Bernd Mathiske


On Jan. 8, 2015, 5:25 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29336/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Recover docker containers that launched in containers.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/docker.hpp b7bf54a 
>   src/slave/containerizer/docker.cpp 5f4b4ce 
> 
> Diff: https://reviews.apache.org/r/29336/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29336/#review77740
-----------------------------------------------------------



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment125937>

    Maybe use the "_" for the parameter, not the local variable? Then both local vars "containers" and "executors" look the same in that regard.
    
    However, see the next issue, fixing which would cancel this one.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29336/#comment125939>

    This type/name scheme is hard to understand. How can Docker::Container* values be containers and executors? 
    
    How about choosing different variable names that make this more clear? 
    
    It seems that
    
    containers -> logContainers
    executors -> executorContainers


- Bernd Mathiske


On Jan. 16, 2015, 5:37 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29336/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2015, 5:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Recover docker containers that launched in containers.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/docker.hpp b7bf54a 
>   src/slave/containerizer/docker.cpp 5f4b4ce 
> 
> Diff: https://reviews.apache.org/r/29336/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29336/
-----------------------------------------------------------

(Updated Jan. 17, 2015, 1:37 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Changes
-------

rebased.


Repository: mesos-git


Description
-------

Recover docker containers that launched in containers.


Diffs (updated)
-----

  src/slave/containerizer/docker.hpp b7bf54a 
  src/slave/containerizer/docker.cpp 5f4b4ce 

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


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29336/
-----------------------------------------------------------

(Updated Jan. 13, 2015, 2:19 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Repository: mesos-git


Description
-------

Recover docker containers that launched in containers.


Diffs (updated)
-----

  src/slave/containerizer/docker.hpp b7bf54a 
  src/slave/containerizer/docker.cpp 5f4b4ce 

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


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29336/
-----------------------------------------------------------

(Updated Jan. 9, 2015, 1:25 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Changes
-------

Rebased


Repository: mesos-git


Description
-------

Recover docker containers that launched in containers.


Diffs (updated)
-----

  src/slave/containerizer/docker.hpp b7bf54a 
  src/slave/containerizer/docker.cpp 5f4b4ce 

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


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 29336: Recover docker containers that launched in containers.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29336/
-----------------------------------------------------------

(Updated Jan. 6, 2015, 10:18 p.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Repository: mesos-git


Description
-------

Recover docker containers that launched in containers.


Diffs
-----

  src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815 
  src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665 

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


Testing
-------

make check


Thanks,

Timothy Chen