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:  tmark
                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 tomek):

 * owner:  tomek => tmark


Comment:

 Replying to [comment:8 tmark]:
 > 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.
 Please create a ticket and then you're welcome to forget it. I thought you
 had learned how to play this game already ;)

 > Yes I have a ticket for  this already.
 Excellent!

 > > '''state_model.h'''
 > > 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.
 Think of this as an API that you provide to consumers of this class. Their
 point of view is more important, simply because of the numbers (there will
 likely be more than 1 derived class).

 > > 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.
 Sure. It was just a comment. Actually, there's nothing you can do about
 even if you tried. If the FSM is designed to go into infinite loop,
 detecting it in a general case is impossible (or at least humanity haven't
 solved that problem yet). See http://en.wikipedia.org/wiki/Halting_problem
 for details.

 > > 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.
 My point was to provide onModelFailure() implementation in the base class,
 but keep it virtual. That way deriving a class will be easier. Most people
 don't care about fancy error handling and the default error handler would
 be sufficient for them. Just one less method to worry about when
 developing new FSM. The default error handler can be very basic - log
 error and possibly throw.

 > > 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.
 I like that new approach. I have one minor comment about it, though (see
 below).

 > > 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.
 I agree with your assessment. I'm sure there are lots of other great
 things we can invent to make this FSM even better, but let's not spend any
 more time on this now. We can get back to this once we finish celebrating
 successful demo.

 '''labeled_value.h'''
 I like the concept. One minor thing that I would like to be fixed
 eventually is a better definition of LabeledValueMap with better duplicate
 control. But let's keep the code as it for now. Right now depending on the
 way a duplicate value is inserted, it will either overwrite the previous
 entry or not be inserted at all. A small @todo will do the trick for now.

 Ok, it seems that only very minor things are left out. Regardless if you
 choose to implement or reject my comments, I don't need to see this ticket
 again.

 Minor comment about ChangeLog: make sure that there are 2 tabs after
 [func] and there are no trailing spaces.

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