----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47381/#review133242 -----------------------------------------------------------
Fix it, then Ship it! LGTM. Mind adding a TODO in the re-registration path? I also relized that if the slave crashes while some task is still queued, we'll send TASK_LOST during reregistration, but we won't try to shutdown the executor in that case. I guess you'are working on it? src/slave/slave.cpp (line 2971) <https://reviews.apache.org/r/47381/#comment197527> Wondering if we should only call shutdown if `executor->isCommandExecutor()` is true? - Jie Yu On May 14, 2016, 12:28 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47381/ > ----------------------------------------------------------- > > (Updated May 14, 2016, 12:28 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-5380 > https://issues.apache.org/jira/browse/MESOS-5380 > > > Repository: mesos > > > Description > ------- > > The agent now shuts down the executor during registration if it does not > have any queued tasks (e.g., framework sent a killTask before > registration). > > Note that if the executor doesn't register at all, it will be cleaned up > anyway after the registration timeout value. > > Also, note that this doesn't handle the case where the agent restarts > after processing the killTask() but before cleaning up the executor. > > > Diffs > ----- > > src/slave/slave.cpp 7c870396b4d6804bfda6169d76d136e95cd839f5 > src/tests/slave_tests.cpp 3ec670aa75790417ec8b7b96cfdb787492b104e1 > > Diff: https://reviews.apache.org/r/47381/diff/ > > > Testing > ------- > > make check > > The new test failed w/o the code change. > > > Thanks, > > Vinod Kone > >
