BIND 10 #3089: Integrate NameTransaction management into D2UpdateMgr

[BIND 10 Development]

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

 * owner:  marcin => tmark


Comment:

 Reviewed commit 78eb8a858925785728cf22052c79f09597d96017

 General comment: now that we have new year we should update copyright
 headers to 2014.

 '''src/bin/d2/d2_messages.mes'''
 DHCP_DDNS_UPDATE_REQUEST_SENT: Spurious space between !''server!'' and
 !'':!''.

 Same for the DHCP_DDBS_UPDATE_RESPONSE_RECEIVED.

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

 }}}

 I wonder if any debug logger message should precede the transaction start:
 {{{
 trans->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.

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

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

 clearDnsUpdateRequest: Spurious dot sign at the end of brief.

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


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

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

 TimedIO class is not documented.

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