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