> On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, lines 1330-1341 > > <https://reviews.apache.org/r/23912/diff/3/?file=643812#file643812line1330> > > > > I would recommend pulling this logic outside 'if (executor == NULL)' to > > #1326 to make it easy to reason about. > > > > also, task ids are globally unique. there shouldn't be multiple > > executors with the same task id. you could actually do a > > CHECK(executorIds.size() == 1) to confirm that. > > Vinod Kone wrote: > this wasn't addressed either. is there another diff coming?
Moving the code outside 'if (executor == NULL)' looks like more control flow to me, not less. There should not be any pending executor if there is an assigned one, which 'if (executor == NULL)' tests. We would then have to remember the result of the check for pending executors and take the same action in case there isn't an assigned one. What am I missing? I am dropping this part of the issue, I suppose. Adding CHECK(executorIds.size() == 1). > On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote: > > src/tests/mesos.cpp, lines 330-335 > > <https://reviews.apache.org/r/23912/diff/3/?file=643814#file643814line330> > > > > were you unable to directly call Slave::* methods here instead of > > having the redirection through unmocked_* methods? > > Vinod Kone wrote: > we expect comments for dropped issues. mind adding a comment? That is correct. Calling Slave::*() results in infinite recursion. > On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote: > > src/tests/slave_tests.cpp, lines 938-957 > > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line938> > > > > To simplify this boiler plate you could use the LaunchTasks action. See > > TerminatingSlaveDoesNotReregister test in this file for an example. > > Vinod Kone wrote: > mind adding a comment? I tried that. Cannot do that here. I need to have a variable that references the task in question. > On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote: > > src/tests/slave_tests.cpp, lines 958-962 > > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line958> > > > > Kill these expectations since an executor won't be launched in this > > test. > > Vinod Kone wrote: > doesnt look like this was fixed. I will take off the 'registered' expectation, but I prefer keeping the second one to make sure the test is tight. That includes that launchTask should not happen by accident. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23912/#review49552 ----------------------------------------------------------- On Aug. 5, 2014, 3:45 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23912/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2014, 3:45 p.m.) > > > Review request for mesos. > > > Bugs: MESOS-947 > https://issues.apache.org/jira/browse/MESOS-947 > > > Repository: mesos-git > > > Description > ------- > > Fixes MESOS-947 "Slave should properly handle a killTask() that arrives > between runTask() and _runTask()". > > Slave::killTask() did not check for task in question combination to be > "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) > and then erroneously assumed that Slave::runTask() had not been executed. The > task was then marked "LOST" instead of "KILLED". But Slave::runTask had > already scheduled Slave::_runTask to follow. Now the entry for being > "pending" is removed, and the task is marked "KILLED", and _runTask gets > informed about this. It checks whether the task in question is currently > "pending" and if it is not, then it infers that the task has been killed and > does not erroneously try to complete launching it. > > > Diffs > ----- > > src/slave/slave.hpp a896bb66db5d8cd27ef02b6498c9db93cb0d525f > src/slave/slave.cpp 1d5691836822c8587e1aa8ed24860a8012c67a6e > src/tests/mesos.hpp 75c66fda2485afa0d4541e710780d90b3411839a > src/tests/mesos.cpp 35c94fa908ad728ea92a7d1bfcbe90d57b1b83d9 > src/tests/slave_tests.cpp e45255a6f699e51bf09397da95a5a11edbabe591 > > Diff: https://reviews.apache.org/r/23912/diff/ > > > Testing > ------- > > Wrote a unit test that reliably created the situation described in the > ticket. Observed that TASK_LOST and the listed log output occurred. This > pointed directly to the lines in killTask() where the problem is rooted. Ran > the test after fixing, it succeeded. Checked the log. It looks like a "clean > kill" now :-) > > > File Attachments > ---------------- > > stout patch > > https://reviews.apache.org/media/uploaded/files/2014/08/05/5f4b6886-9a60-4ceb-ad99-6b5a2f69c870__MESOS-947-stout.patch2 > > > Thanks, > > Bernd Mathiske > >
