BIND 10 #3088: Develop NameRemoveTransaction class for b10-dhcp-ddns

[BIND 10 Development]

#3088: Develop NameRemoveTransaction class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:  Task# 4.3     |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

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

 >
 > '''src/bin/d2/d2_messages.mes'''
 > DHCP_DDNS_FORWARD_REMOVE_RRS_BUILD_FAILURE: Suggest that you put comma
 between %1 and reason.
 >

 Fixed.

 > DHCP_DDNS_REMOVE_FAILED: Typo !''There!''. Should be !''The!''.
 >
 Done.

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

 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.

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

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


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

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

 > readyHandler: !''... state model execution.h!'' - spurious !''h!''?
 >

 Got it.

 > !NameRemoveTransactionPtr: the commentary should say: !''Defines a
 pointer to a !NameRemoveTransaction!''.

 Oops. Fixed.

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


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

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

 > One of the differences found is the word !''exercise!'' instead of
 !''exercised!'' in the !NameRemoveStub. :) You may want to correct this
 typo.

 Got it.
 >
 > 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


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

 >
 > construction test: The alignment of the parameters passed to contructor
 is odd.

 Oops. Silly me.

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

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