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

Reply via email to