mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 58951: Made reservation endpoints tests independent from allocator.
Date Tue, 06 Jun 2017 12:38:39 GMT


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 419 (patched)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line461>
> >
> >     Is this change related to removal of `TestAllocator` or is it a general improvement?
If it is the latter, then please do it in a separate patch.

Since we are not controlling the allocator anymore, other offers could appear so with this
change we need to make this adjustment.


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 460-474 (original), 431-443 (patched)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line473>
> >
> >     Do we need to check both `StatusUpdateMessage` and delivery of status update
to the scheduler?
> >     
> >     Instead, does it maybe make sense to additionaly check the task state of the
task status update delivered to the scheduler?

I focussed this some more. We now explicitly check the task state via `statusUpdate` in the
scheduler; we do not observe the message directly anymore.

Thanks for the suggestion!


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 479-482 (original), 448-450 (patched)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line494>
> >
> >     Could you please explain why you are doing this change?

Again, since we are not controlling the allocator anymore, other offers could appear so with
this change we need to make this adjustment.


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 570-571 (original), 530-532 (patched)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line588>
> >
> >     Ditto. Separate patch?

Ditto as well.


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 628-639 (original), 583-590 (patched)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line647>
> >
> >     Let's capture the status update message and check the state is `TASK_KILLED`.

Applied the same pattern as suggested earlier.


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 640-641 (original)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line659>
> >
> >     This looks like a valuable check. Do we have another test that ensures a similar
thing?

The general resource math of resource recovery is e.g., test here, https://github.com/apache/mesos/blob/7ec3269d51d7d180aa857140097c170c469d7959/src/tests/master_allocator_tests.cpp#L1717-L1761.


> On June 2, 2017, 11:08 a.m., Alexander Rukletsov wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 646-648 (original), 595-597 (patched)
> > <https://reviews.apache.org/r/58951/diff/2/?file=1734443#file1734443line665>
> >
> >     Same question here: why this change in this patch?

Again, since we are not controlling the allocator anymore, other offers could appear so with
this change we need to make this adjustment.


- Benjamin


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


On June 6, 2017, 2:38 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58951/
> -----------------------------------------------------------
> 
> (Updated June 6, 2017, 2:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7388
>     https://issues.apache.org/jira/browse/MESOS-7388
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The tests in the case often require an agent ID. To obtain the ID they
> were using a mock allocator to grab agent ID, but not other operations
> with the allocator were performed.
> 
> Instead we now just capture the SlaveRegisteredMessage in order to
> learn an agent's ID.
> 
> 
> Diffs
> -----
> 
>   src/tests/reservation_endpoints_tests.cpp 505c5421e95378177a7a09f462e5625ffa75cd37

> 
> 
> Diff: https://reviews.apache.org/r/58951/diff/3/
> 
> 
> Testing
> -------
> 
> `make check`, also in repetition.
> 
> While this patch is part of this series since later patches depend on it, it could be
commited independently.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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