You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/01/20 00:32:20 UTC
Re: Review Request 42355: Removed the timeout from the filter.
-----------------------------------------------------------
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.
Summary (updated)
-----------------
Removed the timeout from the filter.
Bugs: MESOS-4302
https://issues.apache.org/jira/browse/MESOS-4302
Repository: mesos
Description (updated)
-------
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 (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 48acde69b1a2f305b568a7e322a58708063dd30a
src/tests/hierarchical_allocator_tests.cpp 9362dd306497ba01e0f387c3862456cdcac6f863
Diff: https://reviews.apache.org/r/42355/diff/
Testing (updated)
-------
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
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 544-560
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line544>
> >
> > 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?
Good suggestions. I'll reword comments and add one more allocation.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 546
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line546>
> >
> > 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?
Yep, makes sense. Thanks!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115583
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > 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?
There is no test for this AFAIK, but it's a good idea to have one. I think we can extend the one I have added.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 79
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202266#file1202266line79>
> >
> > It would be great to isolate this fix, any reason you've included the private addition here rather than in a separate patch?
Yeah, I agree that keeping functional changes separate from cleanups and style changes is a good thing, but here I thought I have the right to meld this change together because I removed one field : ). I'll do it in a separate patch.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 480-481
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line480>
> >
> > 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?
If we place `addSlave` first, we'll also have to place an allocation-await block and some comments about the cluster resources, which is IMHO noise for this particular test. My intention was to start with a certain (i.e. meaningful for this particular test) allocation state in order not to distract a reader with irrelevant allocations. I would suggest we reword the note or remove it altogether if you find it misleading.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 527-528
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line527>
> >
> > 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.
Hence the `ASSERT_GT(flags.allocation_interval.secs(), offerFilter.refuse_seconds());` check: if it ever changes, the test will break. But I can explicitly set the interval in flags instead.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1303
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line1303>
> >
> > Would you mind omitting this change here, so that this patch is focused solely on the fix?
Sure!
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1410-1411
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line1410>
> >
> > 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".
Sure. I think you introduced the term "periodic allocation" in https://reviews.apache.org/r/28815/ : ). I can also clean up all other occurencies in the file.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115583
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 21, 2016, 7:17 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 449-451
> > <https://reviews.apache.org/r/42355/diff/2/?file=1202267#file1202267line449>
> >
> > 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?
I always try to show the reader why we actually cared and wrote the test. I believe a breif description of the problem helps the reader to better understand motivation behind the chosen behaviour.
However I agree that such background information should not substitute the exact description. I'll try to rephrase.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115583
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On Jan. 20, 2016, 9:49 a.m., Qian Zhang wrote:
> > One question: Say allocation interval is 10s, at the time 5s, framework sets a filter with 3s, so with this patch, we will expire the filter 10s (max(10, 3)) later, i.e., at the time 15s. Then at the time of 10s (the next allocation cycle), allocator will not allocate any resources to the framework due to the 10s filter which is good and is the issue that we intend to fix. And then in the time 12, a new slave joins, at this moment, allocator will not allocate any resources to the framework too due to the 10s filter, but maybe the new slave has the resources needed by the framework. So my question is whether this is a reasonable behavior, do we filter too much for the framework in this case?
In this case, do we need to cancel the filter once it has taken effect for one time and last for long enough time?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115315
-----------------------------------------------------------
On Jan. 20, 2016, 7:32 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42355/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 7:32 a.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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 20, 2016, 1:49 a.m., Qian Zhang wrote:
> > One question: Say allocation interval is 10s, at the time 5s, framework sets a filter with 3s, so with this patch, we will expire the filter 10s (max(10, 3)) later, i.e., at the time 15s. Then at the time of 10s (the next allocation cycle), allocator will not allocate any resources to the framework due to the 10s filter which is good and is the issue that we intend to fix. And then in the time 12, a new slave joins, at this moment, allocator will not allocate any resources to the framework too due to the 10s filter, but maybe the new slave has the resources needed by the framework. So my question is whether this is a reasonable behavior, do we filter too much for the framework in this case?
>
> Qian Zhang wrote:
> In this case, do we need to cancel the filter once it has taken effect for one time and last for long enough time?
Your concern is valid and we indeed may filter too much. I wonder how probable is your scenario in real-world setups.
Our intention is "filter for X seconds but at least for one allocation touching filtered agent". What we have here is more of a hack and I'd rather remove `std::max()` in favor of a proper fix, which is allocating on resource recovery (MESOS-3078). Does a TODO I left in the code explan it?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115315
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On Jan. 20, 2016, 9:49 a.m., Qian Zhang wrote:
> > One question: Say allocation interval is 10s, at the time 5s, framework sets a filter with 3s, so with this patch, we will expire the filter 10s (max(10, 3)) later, i.e., at the time 15s. Then at the time of 10s (the next allocation cycle), allocator will not allocate any resources to the framework due to the 10s filter which is good and is the issue that we intend to fix. And then in the time 12, a new slave joins, at this moment, allocator will not allocate any resources to the framework too due to the 10s filter, but maybe the new slave has the resources needed by the framework. So my question is whether this is a reasonable behavior, do we filter too much for the framework in this case?
>
> Qian Zhang wrote:
> In this case, do we need to cancel the filter once it has taken effect for one time and last for long enough time?
>
> Alexander Rukletsov wrote:
> Your concern is valid and we indeed may filter too much. I wonder how probable is your scenario in real-world setups.
>
> Our intention is "filter for X seconds but at least for one allocation touching filtered agent". What we have here is more of a hack and I'd rather remove `std::max()` in favor of a proper fix, which is allocating on resource recovery (MESOS-3078). Does a TODO I left in the code explan it?
>
> Alexander Rukletsov wrote:
> To clarify Qian's concern and my answer: filters are set per-agent basis, so a new agent joining the cluster won't be filtered by any existing filters. However, we indeed may filter longer than asked by a framework, but I think being precise about the filter duration is less important than making the refused resources available for other frameworks.
Yes, I agree it does make sense, thanks for the clarification!
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115315
-----------------------------------------------------------
On Jan. 20, 2016, 7:32 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42355/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 7:32 a.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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 20, 2016, 1:49 a.m., Qian Zhang wrote:
> > One question: Say allocation interval is 10s, at the time 5s, framework sets a filter with 3s, so with this patch, we will expire the filter 10s (max(10, 3)) later, i.e., at the time 15s. Then at the time of 10s (the next allocation cycle), allocator will not allocate any resources to the framework due to the 10s filter which is good and is the issue that we intend to fix. And then in the time 12, a new slave joins, at this moment, allocator will not allocate any resources to the framework too due to the 10s filter, but maybe the new slave has the resources needed by the framework. So my question is whether this is a reasonable behavior, do we filter too much for the framework in this case?
>
> Qian Zhang wrote:
> In this case, do we need to cancel the filter once it has taken effect for one time and last for long enough time?
>
> Alexander Rukletsov wrote:
> Your concern is valid and we indeed may filter too much. I wonder how probable is your scenario in real-world setups.
>
> Our intention is "filter for X seconds but at least for one allocation touching filtered agent". What we have here is more of a hack and I'd rather remove `std::max()` in favor of a proper fix, which is allocating on resource recovery (MESOS-3078). Does a TODO I left in the code explan it?
To clarify Qian's concern and my answer: filters are set per-agent basis, so a new agent joining the cluster won't be filtered by any existing filters. However, we indeed may filter longer than asked by a framework, but I think being precise about the filter duration is less important than making the refused resources available for other frameworks.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115315
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115315
-----------------------------------------------------------
One question: Say allocation interval is 10s, at the time 5s, framework sets a filter with 3s, so with this patch, we will expire the filter 10s (max(10, 3)) later, i.e., at the time 15s. Then at the time of 10s (the next allocation cycle), allocator will not allocate any resources to the framework due to the 10s filter which is good and is the issue that we intend to fix. And then in the time 12, a new slave joins, at this moment, allocator will not allocate any resources to the framework too due to the 10s filter, but maybe the new slave has the resources needed by the framework. So my question is whether this is a reasonable behavior, do we filter too much for the framework in this case?
- Qian Zhang
On Jan. 20, 2016, 7:32 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42355/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 7:32 a.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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115797
-----------------------------------------------------------
Ship it!
- 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/42355/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 1:24 a.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 53d8784a3c4f0467756d7bd924e624ba4585e776
>
> Diff: https://reviews.apache.org/r/42355/diff/
>
>
> Testing
> -------
>
> `make check` on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/
-----------------------------------------------------------
(Updated Jan. 22, 2016, 1:24 a.m.)
Review request for mesos, Ben Mahler and Joris Van Remoortere.
Changes
-------
Ben's comments.
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 (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 53d8784a3c4f0467756d7bd924e624ba4585e776
Diff: https://reviews.apache.org/r/42355/diff/
Testing (updated)
-------
`make check` on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 20, 2016, 2:41 a.m., Guangya Liu wrote:
> > What about inverseOfferFilter?
I don't think it is affected by MESOS-4302 because we do not "inverse re-offer" agents inverse-filtered by frameworks. However, we may consider removing the timeout here as well for brevity and consistency. I'll leave this decision to Joris.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115327
-----------------------------------------------------------
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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 20, 2016, 2:41 a.m., Guangya Liu wrote:
> > What about inverseOfferFilter?
>
> Alexander Rukletsov wrote:
> I don't think it is affected by MESOS-4302 because we do not "inverse re-offer" agents inverse-filtered by frameworks. However, we may consider removing the timeout here as well for brevity and consistency. I'll leave this decision to Joris.
Yes, that's what I want, removing the timeout and keep offer and inverse offer consistent, we can leave this decision to Joris.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115327
-----------------------------------------------------------
On 一月 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 一月 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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115327
-----------------------------------------------------------
What about inverseOfferFilter?
- Guangya Liu
On 一月 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 一月 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
>
>
Re: Review Request 42355: Removed the timeout from the filter.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42355/#review115347
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42355]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>