> On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 382 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line382> > > > > Seems you can indeed use INITIALIZING, and then transition to > > RECOVERING here, no?
Technically yes, but 1) slave always performs recovery and 2) we don't want slave to act on messages from master until it recovers. Hence, I have added guards in most of those functions to skip messages if slave is still in recovery. I didn't see any benefit of keeping the slave in the INITIALIZING state. On the other hand, adding another state means more complexity, because we need to be cognizant of yet another state wherever we are checking states. Makes sense? > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 461 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line461> > > > > Like like you should transition to TERMINATED after this wait() call. Again, this is redundant because the slave dies after this. > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 521 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line521> > > > > This looks strange to me, if we don't transition to REGISTERING, we > > shouldn't be registering.. > > > > A conditional guard on the transition implies the same conditional > > guard for the action. > > > > What was the motivation for the guard? Do we need to have registration > > state separate from the overall slave state? This is the comment I added now in the code: // We guard against the RUNNING state because we don't want // to change the state of a slave in RECOVERING state. Makes sense? > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 548 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line548> > > > > Ditto here on the guard. see above. > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 549 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line549> > > > > Should this be UNREGISTERED? It looks like we're not REGISTERING at > > this point. Again, I don't see a reason for UNREGISTERED state because we don't do anything special between UNREGISTERED and REGISTERING states. To me REGISTERING state means the slave is waiting to be registered with the master. This might be after a disconnection with master or the very first time. There is nothing special, afaict, between these two cases. > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 556 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line556> > > > > Looks like this CHECK will fail based on my above comments regarding > > the conditional transition. Not really. Because we skip registration in doReliableRegistration(), if we are not in REGISTERING state. > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 562 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line562> > > > > If we had a separate registration state, this could then be REGISTERED. > > Does that simplify things? What is the difference between REGISTERED and RUNNING? Fwiw, this is how the states look in Executor too. > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 579 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line579> > > > > Do we want a REREGISTERING state? As mentioned earlier, I don't want to proliferate states unless there is something special about that state. > On April 8, 2013, 5:31 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 601 > > <https://reviews.apache.org/r/10268/diff/1/?file=277662#file277662line601> > > > > Ok, this bit here makes me think we do need separate states for > > (re-)registration, since it's a somewhat independent process. The reason this guard exists is an unfortunate side-effect of delay() not returning a future. Since, doReliableRegistration() is a onReady callback of 'recovered.future()', it might so happen that doReliableRegistration() might be called before _initialized() is called (and the state set to REGISTERING). Does that make sense? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10268/#review18784 ----------------------------------------------------------- On April 3, 2013, 6:26 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10268/ > ----------------------------------------------------------- > > (Updated April 3, 2013, 6:26 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/slave/slave.hpp 2529bf500a3265b10ad4cddde10c2d62a6cdb4a0 > src/slave/slave.cpp 325231458a6883019436e7cc5a37f85f0f5735fa > > Diff: https://reviews.apache.org/r/10268/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
