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 2015/04/17 20:21:45 UTC
Review Request 33318: Fix docker containerizer usage and test.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33318/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Repository: mesos
Description
-------
Fix docker containerizer usage and test.
The docker usage test is failing with the most recent change in including executor resources in docker containerizer.
Diffs
-----
src/slave/containerizer/docker.hpp 6893684e6d199a5d69fc8bba8e60c4acaae9c3c9
src/tests/docker_containerizer_tests.cpp c772d4c836de18b0e87636cb42200356d24ec73d
Diff: https://reviews.apache.org/r/33318/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 33318: Fix docker containerizer usage and test.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33318/#review80541
-----------------------------------------------------------
Ship it!
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/33318/#comment130546>
There is a big global invariant here that we should call out even more explicitly for future readers of the code. That is, we add the initial task resources to the ExecutorInfo so that the initial container we create is big enough since some ExecutorInfo's might not actually add resources. This is an unfortunate wart that got added that we should really clean up at some point (basically, we shouldn't need to have the Slave add the tasks resources to the ExecutorInfo because the Containerizer should just be able to figure that out itself). In the mean time, we should have a better comment and even:
if (task.isSome()) {
CHECK(executor.resources() >= task.get().resources());
}
- Benjamin Hindman
On April 17, 2015, 6:21 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33318/
> -----------------------------------------------------------
>
> (Updated April 17, 2015, 6:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix docker containerizer usage and test.
> The docker usage test is failing with the most recent change in including executor resources in docker containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 6893684e6d199a5d69fc8bba8e60c4acaae9c3c9
> src/tests/docker_containerizer_tests.cpp c772d4c836de18b0e87636cb42200356d24ec73d
>
> Diff: https://reviews.apache.org/r/33318/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 33318: Fix docker containerizer usage and test.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33318/#review80506
-----------------------------------------------------------
Patch looks great!
Reviews applied: [33318]
All tests passed.
- Mesos ReviewBot
On April 17, 2015, 6:21 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33318/
> -----------------------------------------------------------
>
> (Updated April 17, 2015, 6:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix docker containerizer usage and test.
> The docker usage test is failing with the most recent change in including executor resources in docker containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.hpp 6893684e6d199a5d69fc8bba8e60c4acaae9c3c9
> src/tests/docker_containerizer_tests.cpp c772d4c836de18b0e87636cb42200356d24ec73d
>
> Diff: https://reviews.apache.org/r/33318/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>