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:45:05 UTC
Review Request 29327: Add slave id to docker container name prefix.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Repository: mesos-git
Description
-------
Add slave id to docker container name prefix.
Diffs
-----
src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
Diff: https://reviews.apache.org/r/29327/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 9, 2015, 2:50 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.cpp, line 537
> > <https://reviews.apache.org/r/29327/diff/1/?file=798947#file798947line537>
> >
> > This is isolated and far from the condition that leads to it. Better to invert the condition and put this branch first. Then the method ends with the other return statement and without such an isolated statement.
> >
> > if (state.isNone()) {
> > return Nothing();
> > }
> > ... the rest ...
+1!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review67437
-----------------------------------------------------------
On Jan. 6, 2015, 10:14 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 10:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
> src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Till Toenshoff <to...@me.com>.
> On Jan. 9, 2015, 2:50 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.cpp, line 313
> > <https://reviews.apache.org/r/29327/diff/1/?file=798947#file798947line313>
> >
> > This was below 80 chars, was it not?
>
> Timothy Chen wrote:
> It is, but BenH wants all the style to be consistent when it comes to dispatching (he made a similar comment in another file in another review before).
> So either they're all one-line, or all multi.
That is a good to know - thanks for clearifying Tim.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review67437
-----------------------------------------------------------
On Jan. 6, 2015, 10:14 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 10:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
> src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Timothy Chen <tn...@apache.org>.
> On Jan. 9, 2015, 2:50 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.cpp, line 313
> > <https://reviews.apache.org/r/29327/diff/1/?file=798947#file798947line313>
> >
> > This was below 80 chars, was it not?
It is, but BenH wants all the style to be consistent when it comes to dispatching (he made a similar comment in another file in another review before).
So either they're all one-line, or all multi.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review67437
-----------------------------------------------------------
On Jan. 6, 2015, 10:14 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 10:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
> src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review67437
-----------------------------------------------------------
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29327/#comment111461>
This was below 80 chars, was it not?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29327/#comment111462>
This is isolated and far from the condition that leads to it. Better to invert the condition and put this branch first. Then the method ends with the other return statement and without such an isolated statement.
if (state.isNone()) {
return Nothing();
}
... the rest ...
- Bernd Mathiske
On Jan. 6, 2015, 2:14 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 2:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
> src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Timothy Chen <tn...@apache.org>.
> On Jan. 13, 2015, 3:27 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.hpp, line 303
> > <https://reviews.apache.org/r/29327/diff/1/?file=798946#file798946line303>
> >
> > Was there any reason to use the '/' as a separator over something else? I'm wondering if the '/' could cause some filesystem path issue if we ever use container names in the filesystem. I assume we don't because I'm assuming this is all tested, but is there any reason not to use a different separator? If we do use the container name for anything path based later we'd have to change the name again and go through another deprecation cycle. Or perhaps a small comment explaining why you chose this separator please, thank you!
This is fixed in the later commit.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review67795
-----------------------------------------------------------
On Jan. 6, 2015, 10:14 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 10:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
> src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review67795
-----------------------------------------------------------
Ship it!
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29327/#comment111874>
Was there any reason to use the '/' as a separator over something else? I'm wondering if the '/' could cause some filesystem path issue if we ever use container names in the filesystem. I assume we don't because I'm assuming this is all tested, but is there any reason not to use a different separator? If we do use the container name for anything path based later we'd have to change the name again and go through another deprecation cycle. Or perhaps a small comment explaining why you chose this separator please, thank you!
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29327/#comment111873>
s/after 0.21.0/starting with 0.22.0/
- Benjamin Hindman
On Jan. 6, 2015, 10:14 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2015, 10:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
> src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/#review69542
-----------------------------------------------------------
Ship it!
Ship It!
- Bernd Mathiske
On Jan. 16, 2015, 3:21 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29327/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2015, 3:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Add slave id to docker container name prefix.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88
> src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7
>
> Diff: https://reviews.apache.org/r/29327/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/
-----------------------------------------------------------
(Updated April 7, 2015, 12:22 a.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Changes
-------
rebased.
Repository: mesos
Description
-------
Add slave id to docker container name prefix.
Diffs (updated)
-----
src/slave/containerizer/docker.hpp 6893684e6d199a5d69fc8bba8e60c4acaae9c3c9
src/slave/containerizer/docker.cpp f9fb07806e3b7d7d2afc1be3b8756eac23b32dcd
Diff: https://reviews.apache.org/r/29327/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/
-----------------------------------------------------------
(Updated Jan. 16, 2015, 11:21 p.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Repository: mesos-git
Description
-------
Add slave id to docker container name prefix.
Diffs (updated)
-----
src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88
src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7
Diff: https://reviews.apache.org/r/29327/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/
-----------------------------------------------------------
(Updated Jan. 16, 2015, 11:11 p.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Repository: mesos-git
Description
-------
Add slave id to docker container name prefix.
Diffs (updated)
-----
src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88
src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7
Diff: https://reviews.apache.org/r/29327/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29327: Add slave id to docker container name
prefix.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29327/
-----------------------------------------------------------
(Updated Jan. 6, 2015, 10:14 p.m.)
Review request for mesos, Benjamin Hindman and Bernd Mathiske.
Repository: mesos-git
Description
-------
Add slave id to docker container name prefix.
Diffs
-----
src/slave/containerizer/docker.hpp 28ebc6272cd68167fc9a0898fd8eb9d53890b815
src/slave/containerizer/docker.cpp 19a6ea2b5342abe4bf10cf4ea2d5d756df9f8665
Diff: https://reviews.apache.org/r/29327/diff/
Testing
-------
make check
Thanks,
Timothy Chen