----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22796/#review47489 -----------------------------------------------------------
Looks great. Just a few minor nits, and a request for an offer_timeout=0 test, then we can ship it! src/master/flags.hpp <https://reviews.apache.org/r/22796/#comment83343> Mention that offer_timeout=0 means never timeout. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83344> Extra blank line. Use 2 here, not 3. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83345> If you're not modifying the default masterFlags, you can just call StartMaster() and it will CreateMasterFlags() for you. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83346> indent src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83347> Ditto. Just use StartSlave() if you're not modifying the default flags src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83348> StartMaster() src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83350> indent src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83349> StartSlave() src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83352> How about something a little more descriptive than 'message'? src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83351> Times(1) is implicit. You can remove this line. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83358> Weird test. According to your comment below, "We expect rescind to be called from unregister slave". So this comment is inaccurate. And you're not even testing your offer_timeout here then. I'd suggest instead, a test when offer_timeout=0 that the offer is never rescinded. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83353> StartMaster() src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83354> indent src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83355> StartSlave() src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83356> Times(1) is implicit, unnecessary src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83357> indent src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83360> BUG: Declare the FUTURE_PROTOBUF for SlaveRegisteredMessage before you call StartSlave, else it might happen before you setup your future expectation. src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83359> This is the old-school verbose way of launching a task. Find an example of the LaunchTasks() test action in one of these other tests, like SlaveTest.TerminatingSlaveDoesNotReregister src/tests/master_tests.cpp <https://reviews.apache.org/r/22796/#comment83361> Times(1) is implicit, unnecessary - Adam B On July 2, 2014, 5:42 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22796/ > ----------------------------------------------------------- > > (Updated July 2, 2014, 5:42 p.m.) > > > Review request for mesos, Adam B 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 5fef354 > src/master/master.cpp 251b699 > 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 > >
