----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22313/#review50239 -----------------------------------------------------------
src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment87891> I don't think we need to send an update here because when an executor terminates, we send the update for this task (since it is launched tasks) in executorTerminated(). So just return here. src/slave/slave.cpp <https://reviews.apache.org/r/22313/#comment87890> I think you also want to return immediately if executor is in TERMINATING/TERMINATED state. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87892> s/finishing/finishes/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87895> Why do you need to advance the clock? reviveOffers() should do an immediate allocation. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87904> s/updateCall/update/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87903> s/runTask/_runTask/ new line after this. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87906> I think you can get rid of 'runTask' future and just work with 'updateCall' future. Any reason to need both? src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87907> ditto. i don't think you need to advance the clock? src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87908> s/updateCall/update/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87909> Why is the driver.join() here instead of immediately after driver.stop() above? src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87920> Why are you failing the promise here? Don't you want to set it to Nothing and then make sure that task doesn't launch? Otherwise, what you are testing is "containerizer update failure" path and not "executor/framework removed" path. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87921> This should be split up for task1 expectation here and task2 expectation later when that task is launched. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87922> declare status2 where its expectation is declared. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87923> Kill this? Why would there be subsequent updates here? src/tests/slave_tests.cpp <https://reviews.apache.org/r/22313/#comment87941> no need to advance? - Vinod Kone On Aug. 6, 2014, 1:11 a.m., Yifan Gu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22313/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2014, 1:11 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 c12cd0a > src/slave/slave.cpp c56cac8 > src/tests/slave_tests.cpp 3a7fee6 > > Diff: https://reviews.apache.org/r/22313/diff/ > > > Testing > ------- > > SlaveTest.WillNotLaunchTaskBeforeContainerizerUpdate > SlaveTest.WillNotLaunchTaskIfFrameworkIsRemoved > SlaveTest.LaunchTaskAfterContainerizerUpdate > > ./bin/mesos-tests.sh --gtest_repeat=-1 --gtest_break_on_failure > --gtest_filter=*LaunchTask* > > successful times > 2000 > > make check > > > File Attachments > ---------------- > > framework will exit > > https://reviews.apache.org/media/uploaded/files/2014/06/18/fbe73273-7aa9-4faa-b1c5-003ab03042a9__issue-886.diff > log > > https://reviews.apache.org/media/uploaded/files/2014/06/18/84d801a0-5c2a-4bb9-901b-e1962031461c__log > > > Thanks, > > Yifan Gu > >
