BIND 10 #3156: Create separate base-classes for the Finite State Machine logic in NameChangeTransaction class

[BIND 10 Development]

#3156: Create separate base-classes for the Finite State Machine logic in
NameChangeTransaction class
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  low           |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => tomek


Comment:

 Replying to [comment:7 tomek]:
 > General comment: state_model is generic and can be used by other
 > components, not only d2. It should be moved somewhere to src/lib.
 > src/lib/util would be my initial pick, but it should probably be
 > consulted with folks on bind10 chatroom.

 I would prefer to move it once it has matured a bit. Having it close by
 while in the thick of D2 works is simpler.  Moving it later be won't hard
 I just don't want to move it now.

 >
 > This should be reflected by the ChangeLog entry.
 >
 > Please make sure you keep the diagrams, so they can be used for a nice
 doxygen description. I think there's a ticket for that already, isn't it?
 >
 Yes I have a ticket for  this already.
 >
 >
 > '''state_model.h'''
 >
 > File comment mentions nc_trans.h.
 >
 > "No work can done" => "No work can be done"
 >
 > argument which is "posts" => argument which it "posts"
 >
 > "runModel repeats this process until it either NOP_EVT or END_EVT events
 are posted."
 > What about FAIL_EVT?
 >
 > "must wait or ends ." => Extra space before dot.
 >
 > "To progress from from one state" => Extra "from"
 >
 > "a state and an a event" => Remove extra article.
 >
 > "postEvent method is may be" => Either is or may, not both.

 Fixed all of the above.

 >
 > StateModel::SM_STATE_MAX - shouldn't it be SM_STATE_MIN as this is the
 minimal state that derived classes should define?

 Is it half empty or half full?  I was thinking of it as the "maximum value
 a !StateModel state may have".  I did  not change this.

 >
 > runModel(): The fact that state handlers may run multiple interations of
 a state
 > change is a bit worrisome, because it is now impossible to detect
 > potential infinite loops in FSM logic. This brings in a halting
 > problem to the scene. Halting problem is NP-hard, unfortunately. Since
 > I don't have a better solution (if I had, I would likely win Turing
 > Award in the process), let the code stay as it is now.

 Yes, you can certainly hang yourself. Again, I would remind you that I did
 not start out to write a bullet proof FSM implementation.  I have been
 attempting to keep it lightweight, and even if it is a bit "manual" in
 terms of use.  Certainly, we can refine it over time.

 >
 > endModel(): "It is should be called"
 > Either "is called" or "should be called".
 >

 I would like to request a technical writer to do my documentation please.
 ;)
 Fixed.

 > verifyStateHandlerMap():
 > "a implementation" => "an implementation"
 >

 Fixed.


 > onModelFailure(): I think you should provide some generic error handler.
 It would make
 > derived implementations easier.
 >

 I'm not sure I understand.  This let's the derivation react to a model
 failure by doing whatever they need to do, beyond the model transitioning
 to END_ST/FAIL_EVT.  This is invoked when an exception is thrown during
 runModel.  The idea being that the derivation's state model should account
 for ALL errors that may occur, anything that escapes is: an invalid state,
 an invalid event, or an unhandled error.  All of these amount to a
 violation of the model itself and they all are handled from one spot.
 This is something like a "hook" that allows the derivation to do something
 if desired.  It's akin to an "onEntry" or "onExit" concept.


 > getStateLabel(), getEventLabel():
 > It would be better to implement a constructor that takes 2 map<int,
 > string> objects. The methods could be defined in base class and
 > wounldn't have to be defined in each derived class. If there are
 > problems with passing map, then perhaps a static array of
 > {{{
 > struct {
 >     int;
 >     const char*;
 >     StateHandler;
 > }
 > }}}
 > could be passed. That would eliminate the need to implement
 > getStateLabel(), getEventLabel() and probably other as well.
 >
 > addToMap: This name is confusing as it is not clear what the map is,
 > without looking into the doc. addToStateMap or simply addState() would
 > be more appropriate.
 >


 I have replaced the more manual means of specifying events and states with
 a new scheme based upon two new classes: !LabeledValue and
 !LabeledValueSet.  These two let you create a set or dictionary of unique
 numeric values that have labels.  Deriving from these two, lets you expand
 on that.  At the moment an Event is simply a !LabeledValue and they are
 stored in a !LabeledValueSet. A State derives from LabeledValue and then
 adds a handler member. States are stored in a !StateSet which is a
 derivation of !LabeledValueSet.

 StateModel now has two dictionaries, one of valid events and one of valid
 states.  Each level in a StateModel derivation hierarchy may contribute
 events and/or states as before but it is much cleaner now.

 I reworked the unit tests quite a bit.

 > '''state_model_unittests.cc'''
 > Do you think it would be useful to check if runModel() throws if there
 > was no startModel() called?

 This is now tested in the new tests.

 >
 > Also, I think the model should throw if you call addState() after
 > startModel(). Otherwise we could allow a FSM that modifies itself
 > during run. That would encourage shady tricks.
 >

 This is now enforced for both events and states.

 >
 > '''nc_trans.h'''
 > Why do you need NameChangeTransactionError anymore? Wouldn't
 > StateModelError suffice?
 >

 There are errors that can occur in NCT that do not pertain to the state
 model.
 Look at NCT's constructor for instance.


 > verifyStateHandlerMap(): This comment mostly repeats what is said for
 > StateModel::verifyStateHandlerMap(). Perhaps you could trim down the
 > comments in NameChangeTransaction.

 Obsoleted by changes above.

 >
 > If you agree to my proposal for passing map of states as an array or
 > map, then several methods wouldn't be necessary in
 > NameChangeTransaction.

 Addressed by changes above.  As an aside, I had started to go down this
 road before but decided it would likely take more time than I wanted to
 invest.  I am much happier with the overall result now though and
 subsequent derivations will be easier.  Naturally I have included a new
 diagram that depicts the changes:

 [[Image(lvset_state_model_classes.svg)]]

-- 
Ticket URL: <http://bind10.isc.org/ticket/3156#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development