BIND 10 #3089: Integrate NameTransaction management into D2UpdateMgr
BIND 10 Development
do-not-reply at isc.org
Mon Jan 13 18:35:42 UTC 2014
#3089: Integrate NameTransaction management into D2UpdateMgr
-------------------------------------+-------------------------------------
Reporter: tmark | Owner: tmark
Type: enhancement | Status:
Priority: medium | closed
Component: dhcp-ddns | Milestone: DHCP-
Keywords: Task# 6.5 | Kea1.0-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | complete
Estimated Difficulty: 0 | CVSS Scoring:
Total Hours: 31 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 2
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tmark):
* hours: 3 => 2
* status: reviewing => closed
* resolution: => complete
* totalhours: 29 => 31
Comment:
Replying to [comment:11 marcin]:
> Replying to [comment:10 tmark]:
> > 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!
>
> Yup. What was I thinking?! But now that you made a few changes after
review, you should think about updating copyright headers in files
modified:
> - d2_update_mgr.cc
> - dns_client.cc
> - dns_client.h
> - nc_trans.cc
> - d2_update_mgr_unittests.cc
> - dns_client_unittests.cc
> - nc_trans_unittests.cc
I will update them as I edit them in upcoming changes. ;)
>
> >
> >
> > >
> > > '''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?
>
> Hey, reviews are about reading the code. Or I misunderstood something ;)
>
> >
> > >
> > > '''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.
>
> ok.
>
> >
> > >
> > > I wonder if any debug logger message should precede the transaction
start:
> > > {{{
> > > trans->startTransaction();
> > > }}}
> > >
> >
> > Good idea. I added a log statement inside startTransaction.
>
> Thank you! Did you want to add any description for this new message too?
>
> >
> > >
> > > '''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.
>
> The description is good! Just a small typo: This allows a single
DNSClientImpl instance to be used '''in''' for multiple, sequential
IOFetch calls.
>
Corrected.
> >
> > 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.
>
> Perhaps we should file a ticket which will suggest that the
documentation should be updated for Message that the fromWire is a one-
shot deal?o
I have created Trac# 3286 to cover this and updated dns_client.cc
commentary accordingly.
>
> >
> >
> > > 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.
>
> Ok.
>
> >
> > >
> > > '''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.
>
> Just to be clear. How did you know whether timeout of 100 should be
left, not 5 * 1000?
I kept the value (100ms) that is most useful for unit tests. Once it
becomes configurable its default value will be re-aligned with something
more viable for production use.
>
> >
> > >
> > > clearDnsUpdateRequest: Spurious dot sign at the end of brief.
> > >
> >
> > Ok, Eagle-Eye. Got it.
>
> You're welcome QA team.
>
> >
> >
> > > '''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.
>
> ok.
>
> >
> > >
> > > '''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.
>
> Yes, there is no harm. But the comment regarding the purpose if the
function is always useful.
Added a bit more explanation.
>
> >
> > > '''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.
>
> Thanks :)
>
> >
> > >
> > > TimedIO class is not documented.
> > >
> > >
> >
> > What it's not obvious? Done.
>
> Now everything is obvious. :)
Merged Changes in with git 9ff948a169e1c1f3ad9e1bad1568375590a3ef42
Added ChangeLog entry 725 (git 87670dec20b372cb0afca087f1c7181de7f7fe59)
--
Ticket URL: <http://bind10.isc.org/ticket/3089#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list