----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22313/#review45858 -----------------------------------------------------------
src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment80887> The slave cannot be in RECOVERING. See the comment on line #1099. src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment80889> Kill state == RECOVERING. src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment80912> Is this possible? AFAICT, since the task was added to the executor the executor shouldn't be removed between _runTask() and __runTask(). Even if the executor terminates in between, this task should've been marked 'terminated' but not 'completed' (i.e., waiting for an ACK) and hence the executor won't be removed from the framework's map. Since there is a pending executor, the framework shouldn't be removed. So this can be a CHECK_NONTULL(framework) with a comment on why it can be a check. src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment80917> See my comments above. An executor cannot be removed when it has a pending task. src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment80919> also log the executor id, task id and framework id. src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment80920> include the "(future.isFailed() ? future.failure() : "discarded")" here. though, "discarded" here might not be meaningful for frameworks. hmmm. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment80930> I don't see where this test is ensuring that resources are isolated before launching tasks? I would recommend splitting this into 2 tests 1) ensure resources are updated before task is launched 2) containerizer failed causes task lost to be sent. i would also recommend writing a test that ensures that framework and executor are not removed between _runTask and __runTask(). src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment80928> You should set these expectations just before you launch the second task. Also, why "WillRepeatedly"? Are you expecting more than 2 updates? - Vinod Kone On June 11, 2014, 9:32 a.m., Yifan Gu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22313/ > ----------------------------------------------------------- > > (Updated June 11, 2014, 9:32 a.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-886 > https://issues.apache.org/jira/browse/MESOS-886 > > > Repository: mesos-git > > > Description > ------- > > Added __runTask() to wait for the completion of containerizer->update() and > check the result before sending RunTaskMessage. > > > Diffs > ----- > > src/slave/slave.hpp 34687e5 > src/slave/slave.cpp 643c088 > src/tests/slave_tests.cpp 2c8f183 > > Diff: https://reviews.apache.org/r/22313/diff/ > > > Testing > ------- > > SlaveTest, CancelTaskIfContainerizerFails > > Which tests that if the containerizer->update() return a Failure, the task > won't be launched and the scheduler will get TASK_LOST. > > make check > > > Thanks, > > Yifan Gu > >
