BIND 10 #3089: Integrate NameTransaction management into D2UpdateMgr

[BIND 10 Development]

#3089: Integrate NameTransaction management into D2UpdateMgr
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:  Task# 6.5     |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  29            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  3
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin
 * totalhours:  26 => 29


Comment:

 Replying to [comment:8 marcin]:
 > Reviewed commit 78eb8a858925785728cf22052c79f09597d96017
 >
 > General comment: now that we have new year we should update copyright
 headers to 2014.

 Agreed. In my own defense this ticket went into review in 2013. So there!


 >
 > '''src/bin/d2/d2_messages.mes'''
 > DHCP_DDNS_UPDATE_REQUEST_SENT: Spurious space between !''server!'' and
 !'':!''.
 >
 > Same for the DHCP_DDBS_UPDATE_RESPONSE_RECEIVED.

 Got it. How did you even notice this?

 >
 > '''src/bin/d2/d2_update_mgr.cc'''
 > makeTransaction: awkward alignment of the second line here:
 > {{{
 >         trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
 >                                            forward_domain,
 reverse_domain));
 >
 > }}}

 Fixed.

 >
 > I wonder if any debug logger message should precede the transaction
 start:
 > {{{
 > trans->startTransaction();
 > }}}
 >

 Good idea. I added a log statement inside startTransaction.

 >
 > '''src/bin/d2/dns_client.cc'''
 > Declaration of response_:
 > {{{
 > D2UpdateMessagePtr& response_;
 > }}}
 > It would be useful to have some text explaining why we have to store the
 reference to the object. If it circumvents an issue in Message object it
 would be useful to describe the nature of this problem and. If there is
 anything we plan to do about this problem, there could be a reference to a
 ticket.
 >

 I added commentary in dns_client.h to DNSCLientImpl constructor param.

 As to whether we do anything about this or not, I don't know.   It is
 something of a short coming, at
 least from an API standpoint.  Nothing states that Message::fromWire is a
 one-shot deal.  If you try you
 get a segfault or an obscure exception.


 > DNSClient Constructor: if this is a valid condition that the
 response_placeholder is (or should be) NULL, it should be stated in the
 constructor description. Ini particular I am not sure that this part
 remains valid:
 > {{{
 >  Caller is responsible for allocating this object.
 > }}}

 First, I restated the commentary to be accurate.  Secondly, I enforce that
 it be NULL in the !DNSCLientImpl constructor.  This had a minor ripple
 effect in terms of unit testing but as a whole is cleaner.

 >
 > '''src/bin/d2/nc_trans.h'''
 > There is that:
 > {{{
 > +#if 0
 > +    /// @todo  This value will be configurable in the near future, but
 > +    /// until it is there is no way to replace its value.  For now
 > +    /// we will define it to be relatively short, so unit tests will
 > +    /// run within reasonable amount of time.
 >      static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
 > +#else
 > +    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
 > +#endif
 > }}}
 >
 > Is it a leftover from debugging?

 No this is not left over from debugging those I suppose the #if 0 is
 unecessary since the todo is
 there.  I have removed the #if 0 and revised the todo commentary.

 >
 > clearDnsUpdateRequest: Spurious dot sign at the end of brief.
 >

 Ok, Eagle-Eye. Got it.


 > '''src/bin/d2/tests/d2_update_mgr_unittests.cc'''
 > D2UpdateMgrWrapper: The brief description: !''Wrapper class for
 D2UpdateMgr to provide access non-public methods!'' is missing !''to!''
 before !''non-public!''.
 >

 Ok, got it.

 >
 > '''src/bin/d2/tests/nc_test_utils.cc'''
 > Is toHexText used anywhere?
 >
 > Also, I wonder if you could just make use of the
 isc::util::encode::encodeHex and decodeHex functions instead of making
 conversion on your own?
 >

 No, it is not used in any actual tests but I did use it for debugging some
 packet issues during testing.
 I didn't use decodeHex because my function makes nice readable dumps of 16
 bytes per line, not just continous strings of hex digits.

 I left it in for future use and don't see the harm of having it, it's in
 unit test code.

 > '''src/bin/d2/tests/nc_test_utils.h'''
 > isReceivePending could be documented. in fact the class members of
 !FauxServer could be documented at least.

 Boy, I gotta explain everything. lol.  Done.

 >
 > TimedIO class is not documented.
 >
 >

 What it's not obvious? Done.

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