mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 42629: Added tests for offer filters.
Date Fri, 22 Jan 2016 08:29:35 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message