----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28141/#review61986 -----------------------------------------------------------
Thanks for the clean up! Please (in the future) separate these reviews into refactor and then new behavior; it makes it a lot easier to review. Mostly I just want to confirm the behavior around slave restart and recovery, for both state and then with a long-running fetcher. src/slave/containerizer/mesos/containerizer.hpp <https://reviews.apache.org/r/28141/#comment103906> s/isolator/isolators/ src/slave/containerizer/mesos/containerizer.hpp <https://reviews.apache.org/r/28141/#comment103908> This is not checkpointed so what happens if the fetcher is running and the slave is restarted? When it comes back it won't know about the fetcher... Does this rely on the fetcher being run with a pipe on stdin and getting sigpipe when the slave exits? src/slave/containerizer/mesos/containerizer.hpp <https://reviews.apache.org/r/28141/#comment103917> what about using Owned<Container> and dropping the deletes? src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/28141/#comment103909> Is it true that the pid is only checkpointed after the container successfully launched therefore we can assume it's in the RUNNING state? Please add a comment. src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/28141/#comment103910> wrap here (and elsewhere) in CHECK_NOTNULL? src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/28141/#comment103914> Update message to differentiate between unknown and destroyed. src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/28141/#comment103916> It's confusing having a copy of state around. Can you move the updates to each return point where necessary? src/tests/containerizer_tests.cpp <https://reviews.apache.org/r/28141/#comment103920> Is it feasible to add a test that verifies the fetcher is killed under these circumstances? Otherwise, this test would pass if the fetcher was simply ignored and left running. - Ian Downes On Nov. 17, 2014, 4:49 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28141/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2014, 4:49 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-1922 > https://issues.apache.org/jira/browse/MESOS-1922 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/slave/containerizer/mesos/containerizer.hpp 3baea31 > src/slave/containerizer/mesos/containerizer.cpp 562b03b > src/tests/containerizer_tests.cpp a63897b > > Diff: https://reviews.apache.org/r/28141/diff/ > > > Testing > ------- > > Make check. > > > Thanks, > > Timothy Chen > >
