BIND 10 #3089: Integrate NameTransaction management into D2UpdateMgr
BIND 10 Development
do-not-reply at isc.org
Mon Jan 13 13:10:50 UTC 2014
#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: 29 | Feature Depending on Ticket:
| Add Hours to Ticket: 3
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tmark
Comment:
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
>
>
> >
> > '''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.
>
> 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?
>
>
> > 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?
>
> >
> > 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.
>
> > '''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. :)
--
Ticket URL: <http://bind10.isc.org/ticket/3089#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list