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 42355: Removed the timeout from the filter.
Date Thu, 21 Jan 2016 07:17:55 GMT

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


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