BIND 10 #3088: Develop NameRemoveTransaction class for b10-dhcp-ddns
BIND 10 Development
do-not-reply at isc.org
Tue Dec 17 16:19:43 UTC 2013
#3088: Develop NameRemoveTransaction class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
Reporter: tmark | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp-ddns | Milestone: DHCP-
Keywords: Task# 4.3 | Kea1.0-alpha
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 marcin):
* owner: marcin => tmark
Comment:
Reviewed commit 59bd021c72b3e2d9538b534f3ba00ae4b410da62
Replying to [comment:8 tmark]:
> Replying to [comment:7 marcin]:
> > Reviewed commit 70941f1375063c52a43a10831dc9707c6303f55a
> >
> > In general a very good piece of code. I didn't find anything flagrant.
>
> Thank you. You are too kind.
>
> >
> > '''!ChangeLog'''
> > When referring to !''DHCP_DDNS design!'', it may be appropriate to
give a link where this design can be found. I am not sure what the
practice was so far - and what happens if the link has to change for any
reason. On the other hand, referring to the design without saying were it
can be found may not make sense either.
> >
> > Also, please include a ticket and commit number for the changelog
entry.
>
> I did see other links in the !ChangeLog but rather than do that I
reworded it as follows:
>
> {{{
> 7xx. [func] tmark
> Added the initial implementation of the class, NameAddTransaction,
> to b10-dhcp-ddns. This class provides a state machine which
implements
> the logic required to remove forward and reverse DNS entries as
described
> in RFC 4703, section 5.5. This includes the ability to construct the
> necessary DNS requests.
> (trac# 3088 git TBD)
> }}}
>
Looks good.
> >
> > '''src/bin/d2/d2_messages.mes'''
> > DHCP_DDNS_FORWARD_REMOVE_RRS_BUILD_FAILURE: Suggest that you put comma
between %1 and reason.
> >
>
> Fixed.
There are some messages for which you added extra argument !''event!''.
Like here:
{{{
% DHCP_DDNS_ADD_FAILED DHCP_DDNS failed attempting to make DNS mapping
additions for this request: %1, event: %2
}}}
The argument %2 is actually a !''context!'' which consists of current
state and next event. Perhaps you could just replace the word !''event!''
with !''context!''.
>
> > DHCP_DDNS_REMOVE_FAILED: Typo !''There!''. Should be !''The!''.
> >
> Done.
ok
>
> > '''src/bin/d2/nc_add.cc'''
> >
> > verifyEvents, verifyStates: it is good that you expanded NCT
abbrevation to the long form. But I think it was also good to have the
extended description what the verification is about, i.e. that you check
that states and events are defined.
> >
ok.
>
> I added a line to the commentary in the .h and a quick bit in the .cc
for both nc_add and nc_remove. All of this is explained in detail in
StateModel. Tomek actually complained in the review of nc_trans that I
repeated too much base class material in the derived class.
Tomek is not right in this case. :)
>
> > processAddFailedHandler: When the NO_MORE_SERVERS_EVT occurs you log
the same error as in the case of UPDATE_FAILED_EVT. Question: is it always
the case that the occurence of NO_MORE_SERVERS_EVT is an error? For
example: if I don't have any servers configured yet. Is it an error? I am
wondering if the log message should be differentiated for the actual
update falure and for the case when there are no servers to use. What do
you think?
> >
> > BTW, what is the code's behavior if there is no server configured? Is
there a warning message issued somewhere that no servers are configured?
If not, simply priting that update has failed without making a point that
that was because no servers are yet configured may be misleading - or at
least doesn't give a lot of information. :)
> >
>
> Yes, NO_MORE_SERVERS_EVT is always an error. First, Configuration
parsing enforces that domains must have at least one server, and
transaction creation done by D2UpdateManager ensures that there is a
matching domain for each direction requested (forward and/or reverse).
You should not ever have the case where a transaction is attempting to
process against an empty server list, other than programmatic error.
>
> I have added the event label to the log. Keep in mind that the
precipitating error(s) will have been logged just prior to this.
ok.
>
> > '''src/bin/d2/nc_remove.cc'''
> >
> > Constructor: The io_service could be sanity checked for being a NULL
pointer. Same for forward_domain and reverse_domain.
> >
>
> Nice try ;). This is checked in the base class, constructor.
>
See? Some commentary in the derived class is useful ;)
>
> > verifyStates: it would be useful to extend the commentary inside this
function to say which states are verified in the parent class. Apparently,
the common states such as SELECTING_FWD_SERVER_ST are verified in the
parent class, but I wasn't certain until I actually went to the parent
class'es code.
>
> Added commentary.
ok.
>
> >
> > '''src/bin/d2/nc_remove.h'''
> >
> > There is a couple of typos being a result of copy-paste from the
!NameAddTransaction. For example, for the defineEvents function: !''
Removes events defined by !NameRemoveTransaction to the event set!''. It
should be !''... from the event set!''. Please check the whole header for
such errors.
> >
>
> Done.
ok
>
> > readyHandler: !''... state model execution.h!'' - spurious !''h!''?
> >
>
> Got it.
ok
>
> > !NameRemoveTransactionPtr: the commentary should say: !''Defines a
pointer to a !NameRemoveTransaction!''.
>
> Oops. Fixed.
>
ok
> >
> > '''src/bin/d2/nc_trans.cc'''
> >
> > addLeaseAddressRdata: The following line:
> > {{{
> > rrset->addRdata(rdata);
> > }}}
> > could be common for both V4 and V6 case. So, the code could look like
this:
> > {{{
> > dns::rdata::ConstRdataPtr rdata;
> > if (ncr_->isV4()) {
> > rdata.reset(new dns::rdata::in::A(ncr_->getIpAddress()));
> > } else {
> > rdata.reset(new
dns::rdata::in::AAAA(ncr_->getIpAddress()));
> > }
> > rrset->addRdata(rdata);
> > }}}
> >
Thsi appears to be still broken.
> > addPtrRdata: Why don't you just use the smart pointer constructor to
initialize the pointer in the point of declaration:
> > {{{
> > dns::rdata::ConstRdataPtr rdata(new
dns::rdata::generic::PTR(getNcr()->getFqdn()));
> > }}}
> > Otherwise, you call default constructor and then reset the pointer
which has some performance implications. Similar comment applies to
addDhcidRdata.
> >
>
> Good catch. I had started down a different track with this. Changed it.
ok
>
>
> > '''src/bin/d2/nc_trans.h'''
> >
> > retryTransition: the name of the parameter has been changed. Please
change the name in the doxygen documentation too.
>
> Shame on me!
>
ok
> >
> > '''src/bin/d2/tests/nc_remove_unittests.cc'''
> >
> > I compared the !NameAddStub and !NameRemoveStub and it occurs to me
that they are very similar. They differ by the object type being created:
!NameAddTransaction vs !NameRemoveTransaction, but even there, they have
the same constructors. The methods like fakeReponse are identical. I
wonder if it makes sense to unify the test classes.
> >
>
> I looked at this a lot and in the end decided the gain was not worth the
investment in time. The primary reason the stubs are necessary is to be
able to override methods as well as alter scope via "using", so the stubs
must inherit from the class they test. The amount of common code is
actually pretty small and most of it involves invocations of inherited
methods. It just seemed like most of the avenues I explored made things
more complicated not less and I wanted to get the real code up and
working. It is something that could be be revisted but I'm not inclined
to pursue it now.
Ok. Let's leave it as is.
>
> > One of the differences found is the word !''exercise!'' instead of
!''exercised!'' in the !NameRemoveStub. :) You may want to correct this
typo.
>
> Got it.
Did you? It is still there.
> >
> > Also, in the !NameRemoveTransactionTest you decided to use the
constructor to create a transaction object (see makeTransaction4), instead
of JSON (as in case of !NameAddStub). Are you planning to change this for
the !NameAddTransactionTest?
> >
>
> Yes, there is a follow on ticket, trac# 3264 which addresses this. I
was trying to spare you an even larger diff. In fact I event stated my
intentions for this in my submission comments:
>
> {{{
> Some further refactoring has been done in the unit test code. A new
class,
> TransactionTest, was added to nc_test_utils. It serves as a common test
> fixture that can be used throughout NameChangeTransaction class
hierarchy.
> It is currently only used in nc_remove_unittests.cc. Using this class in
> nc_trans_unittests.cc and nc_add_unittests.cc will be done under a
subsequent
> ticket to reduce the amount of work in this ticket.
> }}}
>
>
> It is nice to know you read this stuff.... lol
>
Apparently, the ticket description is long enough to convince me not to
read it. But I read the UML state diagrams :)
>
> > construction test: you may want to implement tests for NULL pointers
being passed to the constructor.
>
> All of these are tested in base class constructor(s).
>
Yep. Shame on me. :)
> >
> > construction test: The alignment of the parameters passed to
contructor is odd.
>
> Oops. Silly me.
The alignment is odd also here:
{{{
// Verify that construction with wrong change type fails.
EXPECT_THROW(NameRemoveTransaction(io_service, ncr,
forward_domain, reverse_domain),
NameRemoveTransactionError);
// Verify that a valid construction attempt works.
ncr->setChangeType(isc::dhcp_ddns::CHG_REMOVE);
EXPECT_NO_THROW(NameRemoveTransaction(io_service, ncr,
forward_domain, reverse_domain));
}}}
>
> >
> > buildRemoveFwdAddressRequest: In this part:
> > {{{
> > ASSERT_NO_THROW(name_remove = makeTransaction4(FORWARD_CHG));
> > (name_remove->buildRemoveFwdAddressRequest());
> > ASSERT_NO_THROW(name_remove->buildRemoveFwdAddressRequest());
> >
> > }}}
> > the second line is supposed to be removed?
> >
>
> Yes, thank you. I do this when the test throws because gtest doesn't
emit
> the exception thrown, just that it threw. So I take out the
ASSERT_NO_THROW
> and rerun it to see the exception.
>
ok
--
Ticket URL: <http://bind10.isc.org/ticket/3088#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list