----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21968/#review44741 -----------------------------------------------------------
Ship it! Looks great! Only minor nits & suggestions remaining. src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79252> "this->" is unnecessary here and elsewhere in this function. src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79253> Should all fit on one line, especially after removing "this->" src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79258> Since you don't really care about using up the entire offer, you can skip parsing the Resources and just call LaunchTasks(DEFAULT_EXECUTOR_INFO, 1, 1, 64, "*") Default SlaveFlags setup cpus:2;mem:1024, so anything less than that is fine. src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79265> Re: "when the master comes back" - The detector.appoint call doesn't actually failover the master, it just notifies the slave that there is a "new" master that it should reregister with. You can probably just cut that section out so the comment reads "Pause the clock here so the slave will not send multiple reregister messages before we change its state to TERMINATING." src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79266> s/we/the master/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79268> s/_/master.get()/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment79269> s/this->// - Adam B On June 2, 2014, 4:02 p.m., Yifan Gu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21968/ > ----------------------------------------------------------- > > (Updated June 2, 2014, 4:02 p.m.) > > > Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-878 > https://issues.apache.org/jira/browse/MESOS-878 > > > Repository: mesos-git > > > Description > ------- > > Added a sentence to check the slave's state in doReliableRegistration() to > make sure that the function returns when the slave's state is TERMINATING. > Also, to write the test, I need to make sure there's no additional > doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and > NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/gmock.hpp daba7e3 > src/slave/slave.cpp 3b10a4c > src/tests/mesos.hpp 9ece77a > src/tests/slave_tests.cpp 80fe3cf > > Diff: https://reviews.apache.org/r/21968/diff/ > > > Testing > ------- > > Slave_tests.cpp:TerminatingSlaveDoesNotReregister. > > > File Attachments > ---------------- > > Using DROP_PROTOBUF() > > https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff > > > Thanks, > > Yifan Gu > >
