You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/01/22 02:24:21 UTC
Review Request 42629: Added tests for offer filters.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42629/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-4302
https://issues.apache.org/jira/browse/MESOS-4302
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
Diff: https://reviews.apache.org/r/42629/diff/
Testing
-------
On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="HierarchicalAllocatorTest.FilterTimeout" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure` passes with the patch and fails without.
`GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Alexander Rukletsov
Re: Review Request 42629: Added tests for offer filters.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 509-515
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line509>
> >
> > Looks like you only need the second advance here? Technically, you are triggering two batch allocations here, which doesn't appear to be the intent?
> >
> > It would mean we need a Clock::settle after calling recoverResources.
I was thinking about the following: if we remove the first advance, can there be a race when `expire()` is processed *after* the second allocation cycle? I think it can if we do not ensure the filter is set *before* the first batch allocation (your addition).
Re: triggering two batch allocations here, it doesn't matter because we are interested only in the next allocation, but I agree it may distract the reader.
Marking as "fixed".
> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 525
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line525>
> >
> > Some unicode character here?
:facepalm:
> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1415
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line1415>
> >
> > What is a Quarantee? ;)
double :facepalm: ...
> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1513-1522
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line1513>
> >
> > This change actually belongs in the previous patch, since your last change breaks this test on its own.
Correct, thanks Ben!
> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1497-1502
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line1497>
> >
> > Let's leave this for now, but it would be great to avoid this assumption and explicitly control the allocation interval duration, no? When you are advancing the clock below twice, you are triggering two batch allocations, when your intention appears to be to trigger only one batch allocation.
> >
> > For now I'll fix this same issue in the OfferFilter test, and I'll leave this one as still triggering two allocations since it's an existing test.
Yeah, may be a bit misleading for the reader. I'll note it down and propose a patch some time in the future.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42629/#review115801
-----------------------------------------------------------
On Jan. 22, 2016, 1:24 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42629/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 1:24 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4302
> https://issues.apache.org/jira/browse/MESOS-4302
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/42629/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
>
> `make check`
>
> `GTEST_FILTER="HierarchicalAllocatorTest.FilterTimeout" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure` passes with the patch and fails without.
>
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42629: Added tests for offer filters.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42629/#review115801
-----------------------------------------------------------
Ship it!
Great tests, thanks! I made some tweaks based on the suggestions below, and adjusted some comments as well.
src/tests/hierarchical_allocator_tests.cpp (lines 458 - 459)
<https://reviews.apache.org/r/42629/#comment176864>
If you can, try to wrap comments to reduce jaggedness, i.e. the following:
```
// We put both frameworks into the same role, but we could also
// have had separate roles; this should not influence the test.
```
Is less jagged than your existing one:
```
// We put both frameworks into the same role, but we could also have had
// separate roles; this should not influence the test.
```
Ditto for the rest of the comments throughout the changes here.
src/tests/hierarchical_allocator_tests.cpp (line 505)
<https://reviews.apache.org/r/42629/#comment176867>
It might be a bit easier to just say "the offer filter" here rather than trying to reference the `offerFilter` variable?
src/tests/hierarchical_allocator_tests.cpp (lines 509 - 515)
<https://reviews.apache.org/r/42629/#comment176856>
Looks like you only need the second advance here? Technically, you are triggering two batch allocations here, which doesn't appear to be the intent?
It would mean we need a Clock::settle after calling recoverResources.
src/tests/hierarchical_allocator_tests.cpp (line 525)
<https://reviews.apache.org/r/42629/#comment176859>
Some unicode character here?
src/tests/hierarchical_allocator_tests.cpp (line 529)
<https://reviews.apache.org/r/42629/#comment176863>
How about s/FilterTimeoutRespected/SmallOfferFilterTimeout/ since the existing name could accurately describe your other OfferFilter test as well?
src/tests/hierarchical_allocator_tests.cpp (line 542)
<https://reviews.apache.org/r/42629/#comment176865>
Explicitly typo
src/tests/hierarchical_allocator_tests.cpp (line 1415)
<https://reviews.apache.org/r/42629/#comment176858>
What is a Quarantee? ;)
src/tests/hierarchical_allocator_tests.cpp (lines 1497 - 1502)
<https://reviews.apache.org/r/42629/#comment176860>
Let's leave this for now, but it would be great to avoid this assumption and explicitly control the allocation interval duration, no? When you are advancing the clock below twice, you are triggering two batch allocations, when your intention appears to be to trigger only one batch allocation.
For now I'll fix this same issue in the OfferFilter test, and I'll leave this one as still triggering two allocations since it's an existing test.
src/tests/hierarchical_allocator_tests.cpp (lines 1513 - 1520)
<https://reviews.apache.org/r/42629/#comment176857>
This change actually belongs in the previous patch, since your last change breaks this test on its own.
- Ben Mahler
On Jan. 22, 2016, 1:24 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42629/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 1:24 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4302
> https://issues.apache.org/jira/browse/MESOS-4302
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 953712149bd951789beb29c72779c4ac65aa48dc
>
> Diff: https://reviews.apache.org/r/42629/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
>
> `make check`
>
> `GTEST_FILTER="HierarchicalAllocatorTest.FilterTimeout" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure` passes with the patch and fails without.
>
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>