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:12 tmark]:
 > 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.
 Ok, thanks.

 > > > > '''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.
 I don't really like that approach, but let it be. In that case please add
 a comment to the NCR class (or better to its default constructor) about
 its intended lifetime. A kind of warning that the object created in that
 way is not really usable until certain fields are set.

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

 Ok. So it seems you have only one comment to update and the code is ready
 to go.

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