----------------------------------------------------------- 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 > >
