You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2017/04/18 15:45:21 UTC
Review Request 58510: Added two tests for hierarchical reservation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/
-----------------------------------------------------------
Review request for mesos and Michael Park.
Bugs: MESOS-7149
https://issues.apache.org/jira/browse/MESOS-7149
Repository: mesos
Description
-------
Added two tests for hierarchical reservation.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
Diff: https://reviews.apache.org/r/58510/diff/1/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 58510: Added two tests for hierarchical
reservation.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review176836
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On April 18, 2017, 8:45 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 8:45 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two tests for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
>
>
> Diff: https://reviews.apache.org/r/58510/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added two tests for hierarchical
reservation.
Posted by Jay Guo <gu...@gmail.com>.
> On June 9, 2017, 2:23 a.m., Neil Conway wrote:
> > src/tests/reservation_tests.cpp
> > Lines 2401 (patched)
> > <https://reviews.apache.org/r/58510/diff/1/?file=1693725#file1693725line2401>
> >
> > I have various minor gripes about how this test is written :)
> >
> > When the clock is paused, I think it is better to first advance the clock to ensure the agent has registered. Then register the framework; we should then get an offer at precisely that point (w/o waiting for batch alloc). As written, advancing the clock for the batch alloc is actually what triggers the agent to register, which is fragile (what if agent registration backoff is > batch alloc interval?).
> >
> > i.e., I'd adjust the test as follows: https://gist.github.com/neilconway/2f11988222cd8fb9583905624cb4ddd5
Thank you for great advice! Although I removed this test per @bmahler's comment, I'm sure this will help me in the future :)
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177346
-----------------------------------------------------------
On April 18, 2017, 11:45 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 11:45 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two tests for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
>
>
> Diff: https://reviews.apache.org/r/58510/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added two tests for hierarchical
reservation.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177346
-----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp
Lines 4615 (patched)
<https://reviews.apache.org/r/58510/#comment250902>
Needs to be updated for the change to the `addFramework` API method.
src/tests/reservation_tests.cpp
Lines 2401 (patched)
<https://reviews.apache.org/r/58510/#comment250906>
I have various minor gripes about how this test is written :)
When the clock is paused, I think it is better to first advance the clock to ensure the agent has registered. Then register the framework; we should then get an offer at precisely that point (w/o waiting for batch alloc). As written, advancing the clock for the batch alloc is actually what triggers the agent to register, which is fragile (what if agent registration backoff is > batch alloc interval?).
i.e., I'd adjust the test as follows: https://gist.github.com/neilconway/2f11988222cd8fb9583905624cb4ddd5
src/tests/reservation_tests.cpp
Lines 2424 (patched)
<https://reviews.apache.org/r/58510/#comment250905>
This should be unnecessary; waiting for the first offer is sufficient.
src/tests/reservation_tests.cpp
Lines 2430 (patched)
<https://reviews.apache.org/r/58510/#comment250903>
Settle should be unnecessary.
- Neil Conway
On April 18, 2017, 3:45 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 3:45 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two tests for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
>
>
> Diff: https://reviews.apache.org/r/58510/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added a test for hierarchical reservation.
Posted by Jay Guo <gu...@gmail.com>.
> On June 10, 2017, 3:22 a.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 4606-4607 (patched)
> > <https://reviews.apache.org/r/58510/diff/1/?file=1693724#file1693724line4606>
> >
> > I was a bit confused about why this test was using quota, then I figured out that this is how you wanted to ensure that the allocation goes to the descendant.
> >
> > We should document this at the top of the test, or here, to describe that we leverage quota to test this.
> >
> > Simpler to understand would be to not use quota and add two agents, expecting one agent to have been allocated to the ancestor and descendant, each. Does that not work?
Since we made changes for both quota and fair share stage of allocation, I'm using quota to make sure we cover both logic paths. I updated the test comments, please take a look.
> On June 10, 2017, 3:22 a.m., Benjamin Mahler wrote:
> > src/tests/reservation_tests.cpp
> > Lines 2399-2401 (patched)
> > <https://reviews.apache.org/r/58510/diff/1/?file=1693725#file1693725line2399>
> >
> > These two tests are very similar, the only difference seems to be that one tests that the reservations are allocated using "fair sharing" between the roles (or at least if you removed quota, that would be the case), and the other just tests that it is allocatable to the role?
> >
> > That seems like a reasonable split to ensure that there is a ReservationTest covering this, but it's only accurate if you remove the quota from the first test (otherwise these tests are essentially the same?)
Dropped the test.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177509
-----------------------------------------------------------
On June 13, 2017, 11:40 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 11:40 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 631e800e6566847e015ea2638cc1c2fe673855be
>
>
> Diff: https://reviews.apache.org/r/58510/diff/2/
>
>
> Testing
> -------
>
> make check
>
> comment out https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild" --gtest_also_run_disabled_tests`
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added a test for hierarchical reservation.
Posted by Michael Park <mp...@apache.org>.
> On June 9, 2017, 12:22 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 4606-4607 (patched)
> > <https://reviews.apache.org/r/58510/diff/1/?file=1693724#file1693724line4606>
> >
> > I was a bit confused about why this test was using quota, then I figured out that this is how you wanted to ensure that the allocation goes to the descendant.
> >
> > We should document this at the top of the test, or here, to describe that we leverage quota to test this.
> >
> > Simpler to understand would be to not use quota and add two agents, expecting one agent to have been allocated to the ancestor and descendant, each. Does that not work?
>
> Jay Guo wrote:
> Since we made changes for both quota and fair share stage of allocation, I'm using quota to make sure we cover both logic paths. I updated the test comments, please take a look.
I would suggest:
```
// This test ensures that resources reserved to ancestor roles can be offered
// to their descendants. Since both quota and fair-share stages perform
// reservation allocations, we use a quota'd role to test both cases.
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177509
-----------------------------------------------------------
On June 13, 2017, 8:40 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 8:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 631e800e6566847e015ea2638cc1c2fe673855be
>
>
> Diff: https://reviews.apache.org/r/58510/diff/2/
>
>
> Testing
> -------
>
> make check
>
> comment out https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild" --gtest_also_run_disabled_tests`
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added a test for hierarchical reservation.
Posted by Michael Park <mp...@apache.org>.
> On June 9, 2017, 12:22 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 4606-4607 (patched)
> > <https://reviews.apache.org/r/58510/diff/1/?file=1693724#file1693724line4606>
> >
> > I was a bit confused about why this test was using quota, then I figured out that this is how you wanted to ensure that the allocation goes to the descendant.
> >
> > We should document this at the top of the test, or here, to describe that we leverage quota to test this.
> >
> > Simpler to understand would be to not use quota and add two agents, expecting one agent to have been allocated to the ancestor and descendant, each. Does that not work?
>
> Jay Guo wrote:
> Since we made changes for both quota and fair share stage of allocation, I'm using quota to make sure we cover both logic paths. I updated the test comments, please take a look.
>
> Michael Park wrote:
> I would suggest:
> ```
> // This test ensures that resources reserved to ancestor roles can be offered
> // to their descendants. Since both quota and fair-share stages perform
> // reservation allocations, we use a quota'd role to test both cases.
> ```
Committed. Please let me know if you want to further update this.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177509
-----------------------------------------------------------
On June 13, 2017, 8:40 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 8:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 631e800e6566847e015ea2638cc1c2fe673855be
>
>
> Diff: https://reviews.apache.org/r/58510/diff/2/
>
>
> Testing
> -------
>
> make check
>
> comment out https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild" --gtest_also_run_disabled_tests`
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added two tests for hierarchical
reservation.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177509
-----------------------------------------------------------
Thanks for adding tests!
src/tests/hierarchical_allocator_tests.cpp
Lines 4598 (patched)
<https://reviews.apache.org/r/58510/#comment251104>
"descendant roles"? Ditto for the test name
src/tests/hierarchical_allocator_tests.cpp
Lines 4606-4607 (patched)
<https://reviews.apache.org/r/58510/#comment251105>
I was a bit confused about why this test was using quota, then I figured out that this is how you wanted to ensure that the allocation goes to the descendant.
We should document this at the top of the test, or here, to describe that we leverage quota to test this.
Simpler to understand would be to not use quota and add two agents, expecting one agent to have been allocated to the ancestor and descendant, each. Does that not work?
src/tests/reservation_tests.cpp
Lines 2399-2401 (patched)
<https://reviews.apache.org/r/58510/#comment251106>
These two tests are very similar, the only difference seems to be that one tests that the reservations are allocated using "fair sharing" between the roles (or at least if you removed quota, that would be the case), and the other just tests that it is allocatable to the role?
That seems like a reasonable split to ensure that there is a ReservationTest covering this, but it's only accurate if you remove the quota from the first test (otherwise these tests are essentially the same?)
- Benjamin Mahler
On April 18, 2017, 3:45 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 3:45 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two tests for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
>
>
> Diff: https://reviews.apache.org/r/58510/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added two tests for hierarchical
reservation.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review176837
-----------------------------------------------------------
Bad review!
Reviews applied: [58510, 58509, 58508, 57516, 58584, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 57166, 56805, 57165, 57164]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot Windows
On April 18, 2017, 3:45 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 3:45 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two tests for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
>
>
> Diff: https://reviews.apache.org/r/58510/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added two tests for hierarchical
reservation.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review172249
-----------------------------------------------------------
Bad review!
Reviews applied: [58510, 58509, 58508, 57516, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 57166, 56805, 57165, 57164]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On April 18, 2017, 3:45 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 3:45 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two tests for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad
>
>
> Diff: https://reviews.apache.org/r/58510/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added a test for hierarchical reservation.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/#review177825
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On June 13, 2017, 8:40 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> -----------------------------------------------------------
>
> (Updated June 13, 2017, 8:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test for hierarchical reservation.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 631e800e6566847e015ea2638cc1c2fe673855be
>
>
> Diff: https://reviews.apache.org/r/58510/diff/2/
>
>
> Testing
> -------
>
> make check
>
> comment out https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild" --gtest_also_run_disabled_tests`
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 58510: Added a test for hierarchical reservation.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/
-----------------------------------------------------------
(Updated June 13, 2017, 11:40 p.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-7149
https://issues.apache.org/jira/browse/MESOS-7149
Repository: mesos
Description
-------
Added a test for hierarchical reservation.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp 631e800e6566847e015ea2638cc1c2fe673855be
Diff: https://reviews.apache.org/r/58510/diff/2/
Testing (updated)
-------
make check
comment out https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild" --gtest_also_run_disabled_tests`
Thanks,
Jay Guo
Re: Review Request 58510: Added a test for hierarchical reservation.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58510/
-----------------------------------------------------------
(Updated June 13, 2017, 11:31 p.m.)
Review request for mesos and Michael Park.
Changes
-------
rebase and address comments.
Summary (updated)
-----------------
Added a test for hierarchical reservation.
Bugs: MESOS-7149
https://issues.apache.org/jira/browse/MESOS-7149
Repository: mesos
Description (updated)
-------
Added a test for hierarchical reservation.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 631e800e6566847e015ea2638cc1c2fe673855be
Diff: https://reviews.apache.org/r/58510/diff/2/
Changes: https://reviews.apache.org/r/58510/diff/1-2/
Testing
-------
make check
Thanks,
Jay Guo