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           |  closed
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |  complete
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * status:  reviewing => closed
 * resolution:   => complete


Comment:

 Replying to [comment:9 tomek]:
 > 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 ;)

 I have created ticket #3175 to cover this.

 >
 > > > '''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).
 >

 I renamed them SM_DERIVED_STATE_MIN, SM_DERIVED_EVENT_MIN and so forth.


 >
 > > > 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.
 >

 I have added an empty implementation to the base class.  This way
 derivations do not have to fuss with it unless they want their behavior.


 >
 > '''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.
 >

 The map member in !LabeledValueMap is private and there are no accessors
 by which you can retrieve it. You must go through one of the two "add"
 methods both of which do enforce uniqueness.


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

 Right I always format it correctly once in the log.


 Changes merged with commit 6e9227b1b15448e834d1f60dd655e5633ff9745c

 ChangeLog Entry #679 added, plus I fixed the clang complaint in
 dhcp6/dhcp_srv.h

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