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

 * owner:  tmark => tomek


Comment:

 Replying to [comment:11 tomek]:
 > 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.

 I have created Ticket 3158 for this.


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



 I understand your issue, but I don't agree.  NCRs are trafficked between
 D2 and its clients (DHCP servers). They are largely passive data objects
 and I think it is completely reasonable to be able to create an "empty"
 container.  The basic mechanism when creating them on the receiving end,
 is to instantiate a "blank" NCR and populate it member by member as the
 corresponding Elements are parsed from JSON.  An "empty" object doesn't
 necessarily mean a "broken" object.



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

 I would prefer to make this improvement under trac3156, which is well
 underway.  This is the ticket that creates refactors the FSM into its own
 class, !StateModel.

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