BIND 10 #3086: Develop NameChangeTransaction base class for b10-dhcp-ddns

[BIND 10 Development]

#3086: Develop NameChangeTransaction base class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:  Task# 4.1     |  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:9 tmark]:
 > Replying to [comment:8 tomek]:
 > > 4. I like those diagrams. Please create src/bin/d2/d2.dox page and
 > > add the diagrams there. Please update doc/devel/mainpage.dox to point
 > > out to it. This new page doesn't have to be big. Couple sentences will
 > > be sufficient. Please try to explain the big picture, rather than the
 > > details. This doc should grow over time an the diagrams you made will
 > > fit in there nicely.
 > Should I start this a separate ticket?
 Yes, please.

 > > 5. There is no ChangeLog entry.
 > Oops. I could have sworn I proposed one.  How's this:
 >
 > 6xx.    [func]  tmark
 >     Added update transaction base class, NameChangeTransaction,
 >     to b10-dhcp-ddns.  This class provides the common structure
 >     and methods to implement the state models described in the
 >     DHCP_DDNS design, plus integration with DNSClient and its
 >     callback mechanism for asynchronous IO with the DNS servers.
 Make sure it also includes ticket # and commit-id. Otherwise it's fine.


 > > '''nc_trans. cc'''
 > > !NameChangeTransaction constructor:
 > >
 > > It is possible that !NameChangeRequest be neither forward nor reverse?
 > > It can be, according to !NameChangeRequest constructor. I'm not
 familiar
 > > with the NCR creation process, but I would think that such a
 > > no-fwd/no-reverse update does not make sense, so it should throw in
 > > NCR constructor. If that is not feasible for whatever reason, the
 > > second best step would be to add a check to !NameChangeTransaction
 ctor
 > > and throw there.
 > NCR's default constructor permits it, so technically it could be.  The
 default constructor is used to start create a blank NCR that gets filled
 in as you un-marshall values from the JSON form (see NCR::fromJSON).  NCR
 does have a validateContent() method which is used after all values are
 populated, and it tests for this condition.  The other NCR constructor
 calls validateContent() directly.
 >
 > So it is technically possible for an NCR to have neither set but you
 would have to do so deliberately and you would have to bypass a good deal
 of the system to get it into a transaction.
 It's not a good interface if it allows you to create an object that is
 broken. The object creation should be delayed until it is known what kind
 of update it is. Please add @todo in its constructor. We will get back to
 it when we have more time. (If I could get a dollar every time we say
 that, I could retire already).

 > > setState(): There should be a sanity check on the state passed.
 > Currently, states are integers so there isn't much of a way to sanity
 check, especially when both the base class and the derived class may
 define states. "sanity" checking is really left to whether or not a state
 value maps to a state handler.  This is checked and caught via
 runStateModel() when it invokes getStateHandler().
 So the check should be done in setState() as well. Right now setState()
 can be called with a bogus value. The fact that it is bogus will only be
 discovered much later.

 > > setForwardChangeCompleted() and setReverseChangeCompleted(). How many
 > > packets can one change generate? I thought only one - forward or
 > > reverse. So those two methods could be merged into
 > > setChangeCompleted().
 >
 > A NameChangeRequest may call for both the forward and reverse updates be
 made.
 > These flags are intended as "post-mortem" flags, so that if transaction
 fails,
 > it is possible to tell which portion(s) worked, if any.
 Ok. That makese sense.

 > I'm not sure if your issue is with the test structure or the class
 implementation itself. What I have done is to add a method to the base
 class, verifyStateHandlerMap, that is called after initStateHandlerMap. It
 is up to the derivation to implement the actual verification.
 >
 > This gives the derivation a chance to validate the map prior to
 executing the state model.   Without a way to know what the list of valid
 states are, there is good way for the base class to implement the
 verification.
 >
 > I think this is a reasonable compromise.  Assuming the FSM
 implementation continues to mature, it is something that could be
 revisited.
 It is. Let's stay with it.

 > > There is not tests for operator(), setDnsUpdateStatus(),
 > > setDnsUpdateResponse(), setForwardChangeCompleted(),
 > > setReverseChangeCompleted(), getPrevState(), getDnsUpdateStatus(),
 > > getForwardChangeCompleted(), and getReverseChangeCompleted().
 > For your viewing pleasure, I have added a test, basicAccessors, which
 should address your concerns.
 Ok, it's cool.

 Hmm, seems this ticket is almost done. If you agree with all my comments,
 please carry on. I don't need to see it again.

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