> 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? > > Vinod Kone wrote: > 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?
Sounds good. > 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. > > Vinod Kone wrote: > Again, this is redundant because the slave dies after this. Ok. > 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? > > Vinod Kone wrote: > 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? Makes sense, but I still think this stems from the fact that registration should be a independent Process / state machine. > 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. > > Vinod Kone wrote: > 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. Based on your comment, it sounds like s/REGISTERING/UNREGISTERED/ would match the semantics and consequently be more intuitive. Again, maybe a TODO to make the registration a separate Process / state machine in efforts to simplify the slave code. > 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? > > Vinod Kone wrote: > What is the difference between REGISTERED and RUNNING? Fwiw, this is how > the states look in Executor too. This was _if_ the registration state was independent, I think it would simplify some of this code quite a lot. Perhaps a TODO if you want to get to it later? Might be a worthwhile investment though vs. doing that refactor later. > 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. > > Vinod Kone wrote: > 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? I think this kind of trickiness can be avoided with separating out the registration process / state machine. > 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? > > Vinod Kone wrote: > As mentioned earlier, I don't want to proliferate states unless there is > something special about that state. This ties in nicely to my s/REGISTERING/UNREGISTERED/ comment above. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10268/#review18784 ----------------------------------------------------------- On April 15, 2013, 6:04 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10268/ > ----------------------------------------------------------- > > (Updated April 15, 2013, 6:04 a.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 > >
