----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22066/#review50377 -----------------------------------------------------------
Thanks Kapil, looks great, just some minor cleanups for the tests. These items are not 'issues' per se, but I've made them issues so that you can fix or drop them as a checklist. I wish ReviewBoard would add a non-"issue" way to tag things in a manner that they tracked as addressed or dropped, but alas! src/master/flags.hpp <https://reviews.apache.org/r/22066/#comment88144> End with a period. :) src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88152> Maybe s/OfferTimeoutNonZero/OfferTimeout/ here and s/OfferTimeoutZero/NoOfferTimeout/ below? Seems a bit clearer? src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88146> Why do you need to wait for this? Seems we could remove the FUTURE_PROTOBUF and AWAIT_READY here. src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88145> s/resourceOffered/resourceOffers/ When we name these Futures satisfied by the callbacks, we'll try to match their name with the callback name. src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88147> Please end comments with a period. :) src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88148> Doesn't look like the Clock::settle and Clock::resume are necessary. Since you are waiting for the offer to be rescinded, you don't need to ensure the pending timers/events are flushed in libprocess via Clock::settle. src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88149> How about expecting that the resources are re-offered to the framework after the rescind? src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88153> How about saying "if there is no rescind timeout" rather than referring to zero? src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88154> How about pluralizing since there are many offers: s/offer1/offers1/ Or per my comment above with matching the callback name: s/offer1/resourceOffers1/ src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88150> Ditto for ending with a period. src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88155> Don't think you need to capture or wait on this future, since you are waiting for the TASK_RUNNING status below. Anything I'm missing here? src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88156> You can be explicit about no rescind occurring by using .Times(0) as you did above. Any reason not to here? src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88151> Period here. src/tests/master_tests.cpp <https://reviews.apache.org/r/22066/#comment88157> I don't think this is enough for this test. We're not sure by the time that we advance the clock whether the offer has been created yet. I would suggest the following: Remove this future, instead wait for the decline LaunchTasksMessage to reach the master, then settle the clock to process it. After this point you can advance it and settle it to ensure that no rescind occurs. Make sense? - Ben Mahler On Aug. 12, 2014, 10:14 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22066/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2014, 10:14 p.m.) > > > Review request for mesos, Adam B, Ben Mahler, Niklas Nielsen, and Timothy > Chen. > > > Bugs: mesos-186 > https://issues.apache.org/jira/browse/mesos-186 > > > Repository: mesos-git > > > Description > ------- > > A timer is associated with each newly created offer. > The offer is rescinded on timeout. > The timer is disarmed on a launchTask or decline. > > This is continuation of Tim's patch: https://reviews.apache.org/r/22796/. > Revision two represents the latest patch from Tim. I have tried to address > the comments/concerns from that request into the third revision. > > > Diffs > ----- > > src/master/flags.hpp 0db4c95 > src/master/master.hpp 29e8f49 > src/master/master.cpp e688b41 > src/tests/master_tests.cpp 9de2424 > > Diff: https://reviews.apache.org/r/22066/diff/ > > > Testing > ------- > > Added one more test after Tim's patch to test offer is not rescinded once it > has been declined. > > make check > > > Thanks, > > Kapil Arya > >
