----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33257/#review80756 -----------------------------------------------------------
src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/33257/#comment130874> Maybe add a TODO here as well to remove this in 0.24? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/33257/#comment130879> maybe s/existing/known/? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/33257/#comment130875> Maybe use LOG(INFO) here because this is quite useful for debugging? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/33257/#comment130876> Ditto on LOG(INFO) src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/33257/#comment130883> Please add a NOTE about the fact that MesosContainerizer will try to recover docker command line executors created by slaves pre 0.23.0. What'll happen if that's the case? src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/33257/#comment130880> Ditto on LOG(INFO). src/slave/slave.cpp <https://reviews.apache.org/r/33257/#comment130870> Any reason just copying the containerType, not the entire containerInfo? Will we need more than just the container type to be checkpointed in the future? I am trying to understand your rationale. To me, it's more understandable to copy the entire containerInfo. src/tests/containerizer_tests.cpp <https://reviews.apache.org/r/33257/#comment130892> s/execId/executorId/ ``` ExecutorID executorId; executorId.set_value(UUID::random().toString()); ContainerID containerId; containerId.set_value(UUID::random().toString()); ``` src/tests/containerizer_tests.cpp <https://reviews.apache.org/r/33257/#comment130893> s/exec/executor/ src/tests/containerizer_tests.cpp <https://reviews.apache.org/r/33257/#comment130895> Do you need to set the frameworkId? src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/33257/#comment130897> Ditto on naming. s/exec/executor/ src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/33257/#comment130898> Set the frameworkId? - Jie Yu On April 17, 2015, 7:07 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33257/ > ----------------------------------------------------------- > > (Updated April 17, 2015, 7:07 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Ian Downes, Jie > Yu, and Till Toenshoff. > > > Bugs: MESOS-2601 > https://issues.apache.org/jira/browse/MESOS-2601 > > > Repository: mesos > > > Description > ------- > > Fixed recover tasks only by the intiated containerizer. > Currently both mesos and docker containerizer recovers tasks that wasn't > started by themselves. > The proposed fix is to record the intended containerizer in the checkpointed > executorInfo, and reuse that information on recover to know if the > containerizer should recover or not. We are free to modify the executorInfo > since it's not being used to relaunch any task. > The external containerizer doesn't need to change since it is only recovering > containers that are returned by the containers script. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp 6893684e6d199a5d69fc8bba8e60c4acaae9c3c9 > src/slave/containerizer/docker.cpp f9fb07806e3b7d7d2afc1be3b8756eac23b32dcd > src/slave/containerizer/mesos/containerizer.cpp > e4136095fca55637864f495098189ab3ad8d8fe7 > src/slave/slave.cpp a0595f93ce4720f5b9926326d01210460ccb0667 > src/tests/containerizer_tests.cpp 5991aa628083dac7c5e8bf7ba297f4f9edeec05f > src/tests/docker_containerizer_tests.cpp > c772d4c836de18b0e87636cb42200356d24ec73d > > Diff: https://reviews.apache.org/r/33257/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
