You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2017/05/30 10:22:10 UTC
Re: Review Request 58951: Made reservation endpoints tests
independent from allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58951/
-----------------------------------------------------------
(Updated May 30, 2017, 12:22 p.m.)
Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
Changes
-------
Rebased.
Summary (updated)
-----------------
Made reservation endpoints tests independent from allocator.
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 (updated)
-----
src/tests/reservation_endpoints_tests.cpp 8c195eb7d788fbca8722d66d81c77d399702160a
Diff: https://reviews.apache.org/r/58951/diff/2/
Changes: https://reviews.apache.org/r/58951/diff/1-2/
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
Re: Review Request 58951: Made reservation endpoints tests
independent from allocator.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58951/#review177156
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On June 6, 2017, 12: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, 12: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
>
>
Re: Review Request 58951: Made reservation endpoints tests
independent from allocator.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed comments from alexr.
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 (updated)
-----
src/tests/reservation_endpoints_tests.cpp 505c5421e95378177a7a09f462e5625ffa75cd37
Diff: https://reviews.apache.org/r/58951/diff/3/
Changes: https://reviews.apache.org/r/58951/diff/2-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
Re: Review Request 58951: Made reservation endpoints tests
independent from allocator.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On June 2, 2017, 9: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?
>
> Benjamin Bannier wrote:
> 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.
Thank you, Benjamin, for searching and finding it!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58951/#review176742
-----------------------------------------------------------
On June 6, 2017, 12: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, 12: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
>
>
Re: Review Request 58951: Made reservation endpoints tests
independent from allocator.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 58951: Made reservation endpoints tests
independent from allocator.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58951/#review176742
-----------------------------------------------------------
src/tests/reservation_endpoints_tests.cpp
Line 374 (original), 349 (patched)
<https://reviews.apache.org/r/58951/#comment250195>
Pass `masterFlags`?
src/tests/reservation_endpoints_tests.cpp
Line 442 (original), 413 (patched)
<https://reviews.apache.org/r/58951/#comment250196>
Do you want to update the comment?
src/tests/reservation_endpoints_tests.cpp
Lines 419 (patched)
<https://reviews.apache.org/r/58951/#comment250197>
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.
src/tests/reservation_endpoints_tests.cpp
Lines 460-474 (original), 431-443 (patched)
<https://reviews.apache.org/r/58951/#comment250198>
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?
src/tests/reservation_endpoints_tests.cpp
Lines 479-482 (original), 448-450 (patched)
<https://reviews.apache.org/r/58951/#comment250199>
Could you please explain why you are doing this change?
src/tests/reservation_endpoints_tests.cpp
Lines 570-571 (original), 530-532 (patched)
<https://reviews.apache.org/r/58951/#comment250200>
Ditto. Separate patch?
src/tests/reservation_endpoints_tests.cpp
Line 609 (original), 565 (patched)
<https://reviews.apache.org/r/58951/#comment250201>
Update the comment as well?
src/tests/reservation_endpoints_tests.cpp
Lines 628-639 (original), 583-590 (patched)
<https://reviews.apache.org/r/58951/#comment250202>
Let's capture the status update message and check the state is `TASK_KILLED`.
src/tests/reservation_endpoints_tests.cpp
Lines 640-641 (original)
<https://reviews.apache.org/r/58951/#comment250203>
This looks like a valuable check. Do we have another test that ensures a similar thing?
src/tests/reservation_endpoints_tests.cpp
Lines 646-648 (original), 595-597 (patched)
<https://reviews.apache.org/r/58951/#comment250204>
Same question here: why this change in this patch?
src/tests/reservation_endpoints_tests.cpp
Lines 1403 (patched)
<https://reviews.apache.org/r/58951/#comment250205>
You don't necessarily need this here. Actually it might even seem misleading because people may think it is doing something.
- Alexander Rukletsov
On May 30, 2017, 10:22 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58951/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 10:22 a.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 8c195eb7d788fbca8722d66d81c77d399702160a
>
>
> Diff: https://reviews.apache.org/r/58951/diff/2/
>
>
> 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
>
>