----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66145/#review199684 -----------------------------------------------------------
src/tests/slave_tests.cpp Lines 4919-4928 (patched) <https://reviews.apache.org/r/66145/#comment280069> You can probably combine these two comments into one comment before the test. Perhaps: "This test ensures that tasks using the same executor are successfully launched in the order in which the agent receives the runTask(Group)Message, even when we manually reorder the completion of the asynchronous unschedule GC step. See MESOS-8624." src/tests/slave_tests.cpp Lines 4940-4943 (patched) <https://reviews.apache.org/r/66145/#comment280073> Could you make use of the `v1::createExecutorInfo` helper here? And move this down closer to where it is used; you can pass the framework ID directly to that helper after subscription is complete. src/tests/slave_tests.cpp Lines 4962 (patched) <https://reviews.apache.org/r/66145/#comment280075> You can eliminate this and instead do something like: ``` EXPECT_CALL(*scheduler, connected(_)) .WillOnce(v1::scheduler::SendSubscribe(v1::DEFAULT_FRAMEWORK_INFO)); ``` src/tests/slave_tests.cpp Lines 4990-5011 (patched) <https://reviews.apache.org/r/66145/#comment280074> Could you make use of the `createTask` and `createTaskGroupInfo` helpers here? src/tests/slave_tests.cpp Lines 5018 (patched) <https://reviews.apache.org/r/66145/#comment280077> Similar to my comment in a previous patch, please expand on this comment to explain why this is done. src/tests/slave_tests.cpp Lines 5061-5063 (patched) <https://reviews.apache.org/r/66145/#comment280079> Could you do `Clock::pause` at the beginning of the test to eliminate the need for the pause and resume here? src/tests/slave_tests.cpp Lines 5067-5068 (patched) <https://reviews.apache.org/r/66145/#comment280080> I think you can remove this. src/tests/slave_tests.cpp Lines 5069-5070 (patched) <https://reviews.apache.org/r/66145/#comment280081> I think you can remove the first sentence of this comment. src/tests/slave_tests.cpp Lines 5073 (patched) <https://reviews.apache.org/r/66145/#comment280082> s/launches/launch/ s/verify/verifies/ - Greg Mann On March 20, 2018, 6:35 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66145/ > ----------------------------------------------------------- > > (Updated March 20, 2018, 6:35 p.m.) > > > Review request for mesos, Chun-Hung Hsiao and Greg Mann. > > > Bugs: MESOS-8617 and MESOS-8624 > https://issues.apache.org/jira/browse/MESOS-8617 > https://issues.apache.org/jira/browse/MESOS-8624 > > > Repository: mesos > > > Description > ------- > > Agent should launch the task in their receiving order. > On the task launch path, there are currently two > asynchronous steps which may complete out of order: > unschedule GC and task authorization. > > This test simulates the reordering of the completions > of unschedule GC step and verify that, despite the > reordering, tasks can still launch in their original order. > > > Diffs > ----- > > src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354 > > > Diff: https://reviews.apache.org/r/66145/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
