mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 42355: Removed the timeout from the filter.
Date Thu, 21 Jan 2016 15:14:44 GMT


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


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