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:51:06 UTC
Review Request 29335: Fix docker container naming and tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Repository: mesos-git
Description
-------
Fix docker container naming and tests.
"." is an illegal character for docker containers, replacing it with "/"
Diffs
-----
src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
src/tests/docker_containerizer_tests.cpp bed2d1005647d62b3ec013315b883d33990b896e
Diff: https://reviews.apache.org/r/29335/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/#review67456
-----------------------------------------------------------
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29335/#comment111490>
This character could be declared at a central place, perhaps inside the below-mentioned function or as a #define preceeding the latter, together with DOCKER_NAME_PREFIX.
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment111481>
At this point, after seeing so many copies of this code, I am fairly convinced that a function for container name construction with parameters slaveID and containerId should be defined somewhere and then called here and everywhere else. Spreading copies of this concatenation around looks brittle.
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment111483>
Here is an opportunity to call the above mentioned containerName function.
- Bernd Mathiske
On Jan. 8, 2015, 5:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29335/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fix docker container naming and tests.
> "." is an illegal character for docker containers, replacing it with "/"
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54a
> src/slave/containerizer/docker.cpp 5f4b4ce
> src/tests/docker_containerizer_tests.cpp 2105ae2
>
> Diff: https://reviews.apache.org/r/29335/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Timothy Chen <tn...@apache.org>.
> On Feb. 14, 2015, 3:31 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/docker.hpp, line 38
> > <https://reviews.apache.org/r/29335/diff/4/?file=824298#file824298line38>
> >
> > s/name/names/ ?
name here is a single container name I'm referring to.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/#review72499
-----------------------------------------------------------
On Jan. 17, 2015, 1:36 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29335/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2015, 1:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix docker container naming and tests.
> "." is an illegal character for docker containers, replacing it with "/"
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54a
> src/slave/containerizer/docker.cpp 5f4b4ce
> src/tests/docker_containerizer_tests.cpp 2105ae2
>
> Diff: https://reviews.apache.org/r/29335/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Till Toenshoff <to...@me.com>.
> On Feb. 14, 2015, 3:31 p.m., Till Toenshoff wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1169-1170
> > <https://reviews.apache.org/r/29335/diff/4/?file=824300#file824300line1169>
> >
> > const ref?
Actually, given the recent experience with const refs to temporals, I think this should stay as is.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/#review72499
-----------------------------------------------------------
On Jan. 17, 2015, 1:36 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29335/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2015, 1:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix docker container naming and tests.
> "." is an illegal character for docker containers, replacing it with "/"
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54a
> src/slave/containerizer/docker.cpp 5f4b4ce
> src/tests/docker_containerizer_tests.cpp 2105ae2
>
> Diff: https://reviews.apache.org/r/29335/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Timothy Chen <tn...@apache.org>.
> On Feb. 14, 2015, 3:31 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/docker.cpp, lines 1025-1027
> > <https://reviews.apache.org/r/29335/diff/4/?file=824299#file824299line1025>
> >
> > How is this change related to the naming scheme update?
This is also fixing tests.
> On Feb. 14, 2015, 3:31 p.m., Till Toenshoff wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 1247-1255
> > <https://reviews.apache.org/r/29335/diff/4/?file=824300#file824300line1247>
> >
> > Why can we get rid of this and the two removed, following hunks (#1238-1241, #1244-1245)?
We don't need to capture stop as we utilize the docker executor to wait for things to finish, and we're not launching any other processes anymore.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/#review72499
-----------------------------------------------------------
On Jan. 17, 2015, 1:36 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29335/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2015, 1:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix docker container naming and tests.
> "." is an illegal character for docker containers, replacing it with "/"
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54a
> src/slave/containerizer/docker.cpp 5f4b4ce
> src/tests/docker_containerizer_tests.cpp 2105ae2
>
> Diff: https://reviews.apache.org/r/29335/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/#review72499
-----------------------------------------------------------
There are some changes in here that do not seem to match the review description. Would you mind taking a look, and if needed, adapt the review description?
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29335/#comment118548>
s/name/names/ ?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29335/#comment118549>
How is this change related to the naming scheme update?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment118550>
const string mame& ?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment118551>
const ref?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment118552>
Why can we get rid of this and the two removed, following hunks (#1238-1241, #1244-1245)?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment118553>
const ref?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29335/#comment118554>
const ref?
- Till Toenshoff
On Jan. 17, 2015, 1:36 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29335/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2015, 1:36 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix docker container naming and tests.
> "." is an illegal character for docker containers, replacing it with "/"
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54a
> src/slave/containerizer/docker.cpp 5f4b4ce
> src/tests/docker_containerizer_tests.cpp 2105ae2
>
> Diff: https://reviews.apache.org/r/29335/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/
-----------------------------------------------------------
(Updated Jan. 17, 2015, 1:36 a.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Changes
-------
rebased.
Repository: mesos-git
Description
-------
Fix docker container naming and tests.
"." is an illegal character for docker containers, replacing it with "/"
Diffs (updated)
-----
src/slave/containerizer/docker.hpp b7bf54a
src/slave/containerizer/docker.cpp 5f4b4ce
src/tests/docker_containerizer_tests.cpp 2105ae2
Diff: https://reviews.apache.org/r/29335/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/#review67623
-----------------------------------------------------------
Ship it!
- Bernd Mathiske
On Jan. 9, 2015, 4:43 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29335/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 4:43 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fix docker container naming and tests.
> "." is an illegal character for docker containers, replacing it with "/"
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54a
> src/slave/containerizer/docker.cpp 5f4b4ce
> src/tests/docker_containerizer_tests.cpp 2105ae2
>
> Diff: https://reviews.apache.org/r/29335/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/
-----------------------------------------------------------
(Updated Jan. 10, 2015, 12:43 a.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Repository: mesos-git
Description
-------
Fix docker container naming and tests.
"." is an illegal character for docker containers, replacing it with "/"
Diffs (updated)
-----
src/slave/containerizer/docker.hpp b7bf54a
src/slave/containerizer/docker.cpp 5f4b4ce
src/tests/docker_containerizer_tests.cpp 2105ae2
Diff: https://reviews.apache.org/r/29335/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/
-----------------------------------------------------------
(Updated Jan. 9, 2015, 1:24 a.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Changes
-------
Rebased
Repository: mesos-git
Description
-------
Fix docker container naming and tests.
"." is an illegal character for docker containers, replacing it with "/"
Diffs (updated)
-----
src/slave/containerizer/docker.hpp b7bf54a
src/slave/containerizer/docker.cpp 5f4b4ce
src/tests/docker_containerizer_tests.cpp 2105ae2
Diff: https://reviews.apache.org/r/29335/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29335: Fix docker container naming and tests.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29335/
-----------------------------------------------------------
(Updated Jan. 6, 2015, 10:18 p.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Repository: mesos-git
Description
-------
Fix docker container naming and tests.
"." is an illegal character for docker containers, replacing it with "/"
Diffs
-----
src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
src/tests/docker_containerizer_tests.cpp bed2d1005647d62b3ec013315b883d33990b896e
Diff: https://reviews.apache.org/r/29335/diff/
Testing
-------
make check
Thanks,
Timothy Chen