----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24754/#review50802 -----------------------------------------------------------
Ship it! This is looking really good! Much cleaner! src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24754/#comment88669> Put 'pull' related function after 'fetch' related function since we fetch first then pull. Same for the implementation. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24754/#comment88667> 'image' is from user, so we need to be careful here. What if the user do: ``` image = '|| rm -rf *' ``` So we need to use the argv version here. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24754/#comment88668> CHECK_NE(0, status.get()); src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24754/#comment88671> Any reason keeps this temp variable? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24754/#comment88673> hum, you've transfer the ownership here to the temp variable 'container'. In fact, why keeping a temp variable here? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24754/#comment88674> Ditto. Probably you should just store a plain pointer and explicitly delete it later. - Jie Yu On Aug. 15, 2014, 11:59 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24754/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2014, 11:59 p.m.) > > > Review request for mesos, Jie Yu and Timothy Chen. > > > Repository: mesos-git > > > Description > ------- > > The DockerContainerier needs to be able to properly clean up Docker > containers, regardless of when they are destroyed. For example, if a > container gets destroyed while we are fetching, we need to not keep > running the fetch, nor should we try and start the Docker > container. For this reason, we've split out the states into: > > FETCHING > PULLING > RUNNING > DESTROYING > > In particular, we made 'PULLING' be it's own state so that we could > easily destroy and cleanup when a user initiated pulling a really big > image but we timeout due to the executor registration timeout. Since > we curently have no way to discard a Docker::run, we needed to > explicitely do the pull (which is the part that takes the longest) so > that we can assume we won't have to wait very long for Docker::run to > complete. > > > Diffs > ----- > > src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81 > src/docker/docker.cpp ebdf22684cb6619a1b603546c2aa25ca4b90afc5 > src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2 > src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280 > src/tests/docker_containerizer_tests.cpp > e0fd62f83387635f503817ced7a592cc3ae6e775 > > Diff: https://reviews.apache.org/r/24754/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
