-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115583
-----------------------------------------------------------


Thanks Alex, code change looks great. Feel free to split the fix and the tests 
into different patches if you like.

Is there also an existing test for an offer filter being larger than the 
allocation timeout?


src/master/allocator/mesos/hierarchical.cpp (line 75)
<https://reviews.apache.org/r/42355/#comment176569>

    It would be great to isolate this fix, any reason you've included the 
private addition here rather than in a separate patch?



src/master/allocator/mesos/hierarchical.cpp (line 938)
<https://reviews.apache.org/r/42355/#comment176570>

    // see MESOS-4302 for more information.



src/master/allocator/mesos/hierarchical.cpp (line 942)
<https://reviews.apache.org/r/42355/#comment176571>

    Don't change it right now, but seems that 'seconds' was an unfortunate 
name, the following would have been clearer?
    
    ```
    timeout = std::max(allocationInterval, timeout);
    ```
    
    Let's keep track of this for later so that we can get to it during the 
allocator cleanups.



src/tests/hierarchical_allocator_tests.cpp (lines 449 - 451)
<https://reviews.apache.org/r/42355/#comment176573>

    Why mention expiration between two consecutive allocations here? The way I 
had been thinking about a test here was that we ensure that a filter always 
expires after the next batch allocation. 
    
    So, is the fact that there are two batch allocations here is just an 
implementation detail as far as testing is concerned?
    
    Would you mind referencing MESOS-4302 so that the next person to look at 
this test can follow the history a little more easily?



src/tests/hierarchical_allocator_tests.cpp (line 452)
<https://reviews.apache.org/r/42355/#comment176572>

    but the test may need adjustment as far as timing is concerned, yes?



src/tests/hierarchical_allocator_tests.cpp (lines 480 - 481)
<https://reviews.apache.org/r/42355/#comment176574>

    Took me some time to figure out why this note is here :)
    
    How about placing the addSlave call before we add the frameworks? Will that 
avoid the need for omitting the allocation here and hence the need for the NOTE?



src/tests/hierarchical_allocator_tests.cpp (lines 527 - 528)
<https://reviews.apache.org/r/42355/#comment176575>

    Why not explcitly set the allocation interval by passing the flags into 
initialize()? It seems a bit fragile to assume 100ms is less than the implicit 
default, which may change.



src/tests/hierarchical_allocator_tests.cpp (lines 544 - 560)
<https://reviews.apache.org/r/42355/#comment176580>

    In the bottom section of this test, I'm not sure folks without our context 
will understand what is being done and what is expected to occur.
    
    Maybe we just write something as simple as the following, for example:
    
    The allocator will ensure that offer filters are removed after at least one 
batch allocation has occurred. Therefore, we expect that after the timeout 
elapses, the filter remains active for the next allocation and the resources 
are sent to framework1.
    
    Then, I'm curious why you don't have a subsequent allocation to verify that 
the offer filter was really removed, is there a test for that?



src/tests/hierarchical_allocator_tests.cpp (line 546)
<https://reviews.apache.org/r/42355/#comment176577>

    Can you use offerFilter.refuse_seconds() here? Or create a 'timeout' 
variable which is used to both set the offerFitlter.refuse_seconds and is 
passed to this advance() call?



src/tests/hierarchical_allocator_tests.cpp (line 1303)
<https://reviews.apache.org/r/42355/#comment176581>

    Would you mind omitting this change here, so that this patch is focused 
solely on the fix?



src/tests/hierarchical_allocator_tests.cpp (lines 1408 - 1409)
<https://reviews.apache.org/r/42355/#comment176582>

    As far as terminology goes, it would be great to consistently refer to 
"batch allocation", otherwise readers may be confused as to whether there is a 
distinction between a "periodic allocation" and a "batch allocation".


- Ben Mahler


On Jan. 19, 2016, 11:32 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42355/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4302
>     https://issues.apache.org/jira/browse/MESOS-4302
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Without the timeout, we rely on filter expiration only. This guarantees
> that filter removal is scheduled after `allocate()` if the allocator is
> backlogged given default parameters are used. Additionally we ensure the
> filter timeout is at least as big as the allocation interval.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 48acde69b1a2f305b568a7e322a58708063dd30a 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9362dd306497ba01e0f387c3862456cdcac6f863 
> 
> Diff: https://reviews.apache.org/r/42355/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