----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22796/#review49026 -----------------------------------------------------------
Looks great now, I had a few notes to clean up the tests a bit. Let me know what you think! Just to confirm, you are running these test with a high number of iterations to ensure they are not flaky, right? src/master/master.hpp <https://reviews.apache.org/r/22796/#comment85859> const & src/master/master.cpp <https://reviews.apache.org/r/22796/#comment85862> Do we need to store the Timers at all? OfferIDs are unique and so this should be correct without storing / canceling Timers. Given that, should we simplify it a bit and remove the Timer storage / cancellation? src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85870> How about s/RescindResourceOffers/OfferTimeoutNonZero/ src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85864> Can you capture the Offers so we can later ensure the full resources are re-offered? E.g. Future<vector<Offer> > offers1; Future<vector<Offer> > offers2; EXPECT_CALL(sched, resourceOffers(&driver, _)) .WillOnce(FutureArg<1>(&offers1)); .WillOnce(FutureArg<1>(&offers2)); ... AWAIT_READY(resourcesRecovered); AWAIT_READY(offers2); ASSERT_EQ(1u, offers2.get().size()); // Ensure the scheduler is re-offered the resources through a new offer. EXPECT_EQ(Resources(offers1.get()[0].resources()), Resources(offers2.get()[0].resources())); src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85865> We'll often try to match these with the callback names so that it's clear what callback we're expecting: s/rescinded/offerRescinded/ src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85868> I'm not sure we need this test, since it's conflating a few things: (1) Frameworks do not receive any events from the master after they have un-registered, so this is not specific to rescinded offers. (2) Framework unregistration means that the framework is fully shutdown in the cluster, at which point any outstanding offers would be invalid (meaning that they are implicitly rescinded). However, we don't send these rescinded notifications to the unregistered framework because we completely stop communicating with the framework after unregistration. Does this make sense? Any reason to keep it? src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85869> /timeout/the rescind timeout/ src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85871> How about s/RescindResourceOffers/OfferTimeoutZero/ It seems this would let us easily compare the test case here vs. the one above based on the naming. What do you think? src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85872> What about making sure that we get a single offer and no more than that after we advance the clock? We can do this using a .WillOnce and a .WillRepeatedly that satisfy two futures. Once the first future is ready, we advance the clock. After the clock is settled we can assert that the second future is still pending. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment85873> Do you want a corollary test? This one: OfferNotRescindedOnceUsed Corollary: OfferNotRescindedOnceDeclined - Ben Mahler On July 29, 2014, 1:08 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22796/ > ----------------------------------------------------------- > > (Updated July 29, 2014, 1:08 a.m.) > > > Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen. > > > Bugs: MESOS-186 > https://issues.apache.org/jira/browse/MESOS-186 > > > Repository: mesos-git > > > Description > ------- > > Based on Kapil's patch (https://reviews.apache.org/r/22066/), adding timeout > for each offer from master to remove the offer when it's no longer used. > > > Diffs > ----- > > src/master/flags.hpp 32704ce > src/master/master.hpp d8a4d9e > src/master/master.cpp 273a516 > src/tests/master_tests.cpp 5a1cf7f > > Diff: https://reviews.apache.org/r/22796/diff/ > > > Testing > ------- > > Added three more unit tests from Kapil's patch: Testing offer not rescinded > after task launched, offer not rescinded when framework/slave unregistered. > The test exposed a race condition that can lead to a segfault if two remove > offers are called on the same offer. > > make check. > > > Thanks, > > Timothy Chen > >
