----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25867/#review54480 -----------------------------------------------------------
Ship it! src/master/master.cpp <https://reviews.apache.org/r/25867/#comment94638> Generally I get a bit worried about these versioned TODO's unless we have tickets with the correct Target/Fix Version to make sure we follow up. Will you create tickets when this lands? Otherwise we'll most definitely forget! src/master/master.cpp <https://reviews.apache.org/r/25867/#comment94625> Whoops: s/TOOD/TODO/ src/master/master.cpp <https://reviews.apache.org/r/25867/#comment94627> Should this be 'connected' instead of 'registered'? The slave is still registered when we consider it not connected here. src/master/master.cpp <https://reviews.apache.org/r/25867/#comment94626> You don't need the `->self()` here and below, dispatch can take a pointer to do it for you. src/messages/messages.proto <https://reviews.apache.org/r/25867/#comment94637> Ditto above, isn't "connected" the right word here? src/slave/slave.hpp <https://reviews.apache.org/r/25867/#comment94631> Should this be removed in 0.23.0 per our conversation? src/slave/slave.cpp <https://reviews.apache.org/r/25867/#comment94632> Remove in 0.23.0? src/slave/slave.cpp <https://reviews.apache.org/r/25867/#comment94635> This should be s/deactivated/disconnected/ right? Since we're talking about the sockets (link())? src/slave/slave.cpp <https://reviews.apache.org/r/25867/#comment94636> Ditto about s/deactivated/disconnected/ src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94642> Need this? src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94644> Need this? src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94643> Need this? src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94640> Don't need? src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94641> Don't need? src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94645> src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94646> Phrased a bit oddly, how about: "... deactivate the slave. The slave should not ..." src/tests/partition_tests.cpp <https://reviews.apache.org/r/25867/#comment94647> Couldn't you just expect the dispatch on SlaveObserver::disconnect, instead of the allocator dispatch + clock::settle? - Ben Mahler On Sept. 22, 2014, 11:51 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25867/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2014, 11:51 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-1668 > https://issues.apache.org/jira/browse/MESOS-1668 > > > Repository: mesos-git > > > Description > ------- > > Embeded slave registration status in ping message to solicit slave > re-registration during one way master --> slave partition. > > > Diffs > ----- > > src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 > src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062 > src/messages/messages.proto 7cb3ce651997c04ef1ef95539098ed2a99270b11 > src/slave/slave.hpp 4f3df5c49a8cf72fc7153158c9eb045196b6cf13 > src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e > src/tests/partition_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/25867/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
