You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/03/19 18:42:30 UTC
Review Request 66145: Added a test to verify that task launch order is
enforced.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Bugs: MESOS-8624
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/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/#review199473
-----------------------------------------------------------
PASS: Mesos patch 66145 was successfully built and tested.
Reviews applied: `['66118', '66119', '66120', '65679', '66126', '66143', '66144', '66145']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66145
- Mesos Reviewbot Windows
On March 19, 2018, 6:42 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66145/
> -----------------------------------------------------------
>
> (Updated March 19, 2018, 6:42 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/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/#review200525
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On April 3, 2018, 10:08 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66145/
> -----------------------------------------------------------
>
> (Updated April 3, 2018, 10:08 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 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
>
>
> Diff: https://reviews.apache.org/r/66145/diff/8/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/
-----------------------------------------------------------
(Updated April 5, 2018, 5:35 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
set expectations in the right order.
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 (updated)
-----
src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
Diff: https://reviews.apache.org/r/66145/diff/10/
Changes: https://reviews.apache.org/r/66145/diff/9-10/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/
-----------------------------------------------------------
(Updated April 5, 2018, 3:38 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Rebase.
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 (updated)
-----
src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
Diff: https://reviews.apache.org/r/66145/diff/9/
Changes: https://reviews.apache.org/r/66145/diff/8-9/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/
-----------------------------------------------------------
(Updated April 3, 2018, 3:08 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Relaxed GC module expectation calls to avoid rare test failure.
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 (updated)
-----
src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
Diff: https://reviews.apache.org/r/66145/diff/6/
Changes: https://reviews.apache.org/r/66145/diff/5-6/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On April 3, 2018, 10:40 a.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 4969 (patched)
> > <https://reviews.apache.org/r/66145/diff/4/?file=1985629#file1985629line4969>
> >
> > Is this necessary?
To avoid uninteresting mock call.
> On April 3, 2018, 10:40 a.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 5049-5051 (patched)
> > <https://reviews.apache.org/r/66145/diff/4/?file=1985629#file1985629line5049>
> >
> > Should we confirm here that the second task has not launched yet?
> >
> > i.e.
> > ```
> > ASSERT_TRUE(taskStarting2.isPending());
> > ```
> >
> >
> > Also, enclose `taskGroup2` in backticks, here and below.
Good point.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/#review200376
-----------------------------------------------------------
On March 21, 2018, 2:57 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66145/
> -----------------------------------------------------------
>
> (Updated March 21, 2018, 2:57 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 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
>
>
> Diff: https://reviews.apache.org/r/66145/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/#review200376
-----------------------------------------------------------
src/tests/slave_tests.cpp
Lines 4930 (patched)
<https://reviews.apache.org/r/66145/#comment281087>
I think the following name is a little easier to read:
s/TasksLaunchReorderUnscheduleGC/LaunchTasksReorderUnscheduleGC/
src/tests/slave_tests.cpp
Lines 4938 (patched)
<https://reviews.apache.org/r/66145/#comment281088>
Let's move this declaration/assignment down so it's directly above where it first gets used in the `EXPECT_CALL`.
src/tests/slave_tests.cpp
Lines 4940-4943 (patched)
<https://reviews.apache.org/r/66145/#comment281090>
Be consistent here for the creation of the agent dependencies: let's either have a newline between each line, or no newlines at all.
src/tests/slave_tests.cpp
Lines 4969 (patched)
<https://reviews.apache.org/r/66145/#comment281091>
Is this necessary?
src/tests/slave_tests.cpp
Lines 5045-5046 (patched)
<https://reviews.apache.org/r/66145/#comment281094>
Usually when only one word of a comment runs onto the next line, i'll split a bit earlier to improve readability. Also, need a period at the end of the comment:
```
// Reorder the task group launches by resuming
// the processing of `taskGroup2` first.
```
src/tests/slave_tests.cpp
Lines 5049-5051 (patched)
<https://reviews.apache.org/r/66145/#comment281092>
Should we confirm here that the second task has not launched yet?
i.e.
```
ASSERT_TRUE(taskStarting2.isPending());
```
Also, enclose `taskGroup2` in backticks, here and below.
src/tests/slave_tests.cpp
Lines 5055 (patched)
<https://reviews.apache.org/r/66145/#comment281096>
s/subseuqently/subsequently/
- Greg Mann
On March 21, 2018, 9:57 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66145/
> -----------------------------------------------------------
>
> (Updated March 21, 2018, 9:57 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 028cd32c7043eba4e6f2045956471bd0bf42371c
>
>
> Diff: https://reviews.apache.org/r/66145/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/#review200382
-----------------------------------------------------------
src/tests/slave_tests.cpp
Lines 4927 (patched)
<https://reviews.apache.org/r/66145/#comment281098>
s/runTask(Group)Message/RunTask(Group)Message/
- Greg Mann
On March 21, 2018, 9:57 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66145/
> -----------------------------------------------------------
>
> (Updated March 21, 2018, 9:57 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 028cd32c7043eba4e6f2045956471bd0bf42371c
>
>
> Diff: https://reviews.apache.org/r/66145/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/
-----------------------------------------------------------
(Updated March 21, 2018, 2:57 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Thanks for the review! The test looks much concise now.
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 (updated)
-----
src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354
Diff: https://reviews.apache.org/r/66145/diff/3/
Changes: https://reviews.apache.org/r/66145/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 66145: Added a test to verify that task launch
order is enforced.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66145/
-----------------------------------------------------------
(Updated March 20, 2018, 11:35 a.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Updated the test to use mock garbage collector to simulate the reordering.
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 (updated)
-----
src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354
Diff: https://reviews.apache.org/r/66145/diff/2/
Changes: https://reviews.apache.org/r/66145/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu