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 42629: Added tests for offer filters.
Date Fri, 22 Jan 2016 13:26:23 GMT


> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 509-515
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line509>
> >
> >     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.

I was thinking about the following: if we remove the first advance, can there be a race when
`expire()` is processed *after* the second allocation cycle? I think it can if we do not ensure
the filter is set *before* the first batch allocation (your addition).

Re: triggering two batch allocations here, it doesn't matter because we are interested only
in the next allocation, but I agree it may distract the reader.

Marking as "fixed".


> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 525
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line525>
> >
> >     Some unicode character here?

:facepalm:


> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1415
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line1415>
> >
> >     What is a Quarantee? ;)

double :facepalm: ...


> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1513-1522
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line1513>
> >
> >     This change actually belongs in the previous patch, since your last change breaks
this test on its own.

Correct, thanks Ben!


> On Jan. 22, 2016, 8:29 a.m., Ben Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 1497-1502
> > <https://reviews.apache.org/r/42629/diff/1/?file=1205229#file1205229line1497>
> >
> >     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.

Yeah, may be a bit misleading for the reader. I'll note it down and propose a patch some time
in the future.


- Alexander


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


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