BIND 10 #3087: Develop NameAddTransaction class for b10-dhcp-ddns

[BIND 10 Development]

#3087: Develop NameAddTransaction class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:  Task# 4.2     |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131204
         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:10 marcin]:
 > Reviewed commit: e6df9bca72feedd92ec08dc43b53dfec632f228c
 >
 > This is a very good work. It nicely fits to the diagrams appended to the
 ticket which is an indication of both that the design is strictly followed
 in the code and that the code is easier to read having the diagrams :)
 >
 > Also, there is a lot of commentary, and almost every function is
 explained very clearly.

 Thank you, kindly.

 >
 > The review follows:
 >
 > '''!ChangeLog'''
 > Please make sure that the changelog formatting (including tabs and
 indents in general) fits the format of other entries.
 >

 Yes, I will:

 {{{
 6xx.    [func]  tmark

         Added the initial implementation of the class, NameAddTransaction,
         to b10-dhcp-ddns.  This class provides the state model logic
 described
         in the DHCP_DDNS design to add or replace forward and reverse DNS
 entries
         for a given FQDN.  It does not yet construct the actual DNS update
         requests, this will be added under Trac# 3241.

         (Trac# 3207, git TBD)
 }}}


 > '''src/bin/d2/d2_messages.mes'''
 > IMHO, the log messages would read better if their structure was the
 following: <message_id> <what> <when> instead of <message_id> <when>
 <what>. So, for example:
 > {{{
 > DHCP_DDNS_FORWARD_ADD_RESP_CORRUPT DHCP_DDNS received a corrupt
 response, while attempting a request to add a forward address mapping to
 DNS server, %1, for FQDN, %2
 > }}}
 > Usually you're mostly interested what happened and when is less
 important.

 I rearranged them as you suggested.  I agree they are clearer this way.

 >
 > Now, a few typos in the individual messages...
 >
 > - DHCP_DDNS_FORWARD_ADD_REJECTED: typo !''itwas!''

 Got it.

 >
 > - DHCP_DDNS_FORWARD_ADD_IO_ERROR: while line wrap before !''against!''
 and !''the same server!''?

 Got it.

 >
 > - DHCP_DDNS_FORWARD_ADD_RESP_CORRUPT: typo in !''to a update request!'',
 should be !''to an update request!''

 Fixed.


 >
 > - DHCP_DDNS_FORWARD_ADD_UNKNOWN_FAILURE: similar as above: should be
 !''an unexpected!''.

 I changed this id to DHCP_DDNS_FORWARD_ADD_BAD_DNSCLIENT_STATUS which is
 more accurate plus fixed wording.

 >
 > - DHCP_DDNS_FORWARD_REPLACE_REJECTED: !''itwas!'' again. Also, why line
 wrap in the last line?
 >

 Fixed.

 > - DHCP_DDNS_REVERSE_REPLACE_IO_ERROR: Why last line wrap?

 Oops.

 >
 > - DHCP_DDNS_TRANS_SEND_EROR: shouldn't it be !''a DNS update!'', not
 !''an DNS update!''?
 >

 Fixed.

 > '''src/bin/d2/d2_queue_mgr.cc'''
 > '''src/bin/d2/d2_update_mgr.cc'''
 > There are no unit tests to check behaviour when IOService pointer is
 NULL.


 Added unit tests.

 >
 > '''src/bin/d2/dns_client.cc'''
 > '''src/bin/d2/dns_client.h'''
 > Tomek would probably say that !''this change is an abuse of pointers!''.
 :) Even if he didn't, I still claim it is. There is no technical reason
 why IOService needs to be a pointer because the IOService is expected to
 be always initialized for this object. There are some cases when we tend
 to use pointers, not references, to differentiate between the cases when
 object is uninitialized vs initialized. But again, this is not the case
 here. Instead, using pointers here has the following drawbacks:
 > - Have to check for NULL IOService object being passed.
 > - Have to write additional unit tests.
 > - Additional reference counting on shared pointer is performed.
 > - New header had to be added.
 >
 > All in all I don't understand why the IOService has to be represented by
 a pointer. I see the drabacks, but I don't see any benefits. As long as
 this code is unit tested I will not argue about this change. If you want
 to leave it as is, fine. I am just afraid, that writing unit tests will
 take as much time as rolling it back, but I leave it up to you.
 >

 I have reverted changes to DNSClient.


 > '''src/bin/d2/nc_add.cc'''
 > addingFwdAddrsHandler: The IO_COMPLETED_EVT event is handled by this
 function (in fact, not only by this function) but this event is not
 emitted from within the class. It would be useful to have a reference to
 the place in the code where event is triggered. Some little comment which
 refers to NameChangeTransaction::operator() would do the trick.
 >

 Commentary added, and then repeated for the forward and reverse replace
 handlers.

 > '''src/bin/d2/nc_add.h'''
 > readyHandler: Typo !''START_EVT.EADY_ST!''.

 Fixed.

 >
 > readHandler: The brief description seems to be truncated.
 >

 Fixed.

 > replacingFwdaddrsHandler: The brief description seems to be truncated.
 >
 > '''src/bin/d2/nc_trans.cc'''
 > '''src/bin/d2/nc_trans.h'''
 > For certain functions you have left the @todo instead of the actual
 documentation for the functions and members:
 > - sendUpdate
 > - verifyTransition
 > - retryTransition
 > - update_attempts_

 I noticed these after I posted this for review, oops. I guess they were
 "todidn'ts".  Fixed.

 I have left commentary off of buildFwdAddressRequest,
 buildReplaceFwdAddressRequest, and buildReplaceRevPtrsRequest.  This is
 taken care of 3241 which is done and waiting to added after this ticket is
 complete.


 >
 > The argument of the setUpdateAttempts could be passed as a const. Same
 for retryTransition.

 Fixed.

 >
 > clearDnsUpdateResponse: Typo in brief: !''respons!''.

 Fixed.

 >
 > setUpdateAttempts: Question... It seems to me that the only use case for
 the update_attempts_ counter is that you increment it by 1. I wonder if
 you could provide the method incrementUpdateAttempts instead of
 setUpdateAttempts, which would provide more restricted write access to the
 variable. Does it make sense?

 Perhaps, except that setUpdateAttempts is only there for unit test
 purposes. I hadn't planned to expose it all except for test purposes.

 >
 > '''src/bin/d2/tests/nc_add_unittests.cc'''
 >
 > The commentary for functions inside of !NameAddStub could be extended.
 At least mention what fakeResponse, selectFwdServer and selectRevServer is
 doing. Especially, the fakeResponse deserves the commentary.
 >
 > NameAddTransactionTest::makedCannedTransaction: Please extend the
 comments inside of this function. I am not clear wha the !''canned!'' term
 means. I am not clear, NCR is created from JSON, not using a constructor.
 >
 > Also, there is quite a lot of conditional code that is not commented on.
 >

 Addressed.

 > NameAddTransactionTest::prepHandlerTest lacks description.

 Addressed.

 >
 > General comments to all the test: it is probably the only test suite
 where we use underscores to name tests. Is it really neccesary?
 >

 I compromised and removed all but one underscore on the handler tests.
 The
 tests are all named <hander>_<scenario>.  It helps me keep track this way.

 > readyHandler: In the test description !''3. Posted event is invalid!''
 should be !''4. Posted event is invalid!''.
 >

 Fixed.

 > I am pretty confident that this part of unit test is invalid:
 > {{{
 >     // Create a transaction which includes both a forward and a reverse
 change.
 >     ASSERT_NO_THROW(name_add =
 >                     prepHandlerTest(NameChangeTransaction::READY_ST,
 >                                     StateModel::START_EVT,
 FORWARD_CHG));
 >     // Run readyHandler.
 >     EXPECT_NO_THROW(name_add->readyHandler());
 >
 >     // Verify that a request requiring both forward and reverse, starts
 with
 >     // the forward change by transitioning to selecting a forward
 server.
 >     EXPECT_EQ(NameChangeTransaction::SELECTING_FWD_SERVER_ST,
 >               name_add->getCurrState());
 >     EXPECT_EQ(NameChangeTransaction::SELECT_SERVER_EVT,
 >               name_add->getNextEvent());
 > }}}
 > The comment says you should be doing both forward and reverse update,
 while you seem to be doing forward only. Perhaps it is as simple as
 applying the appropriate !''change_mask!'' in the call to prepHandlerTest.
 >

 Good catch! Yes, it was just the wrong mask.


 > selectingFwdServerHandler: Typo !''the a!'' in:
 > {{{
 >     // Post a server IO error event.  This simulates an IO error
 occuring
 >     // and a need to select the a new server.
 >
 > }}}

 Fixed.

 >
 > For completeness of this test I would suggest that the list of servers
 contains two (not just one). This will allow to test that the transaction
 doesn't enter the PROCESS_TRANS_FAILED_ST always when the IO error occurs,
 but it rather loops between two states until IO error doesn't show up or
 the list gets exhausted.
 >

 I have extended the list of servers and the test as suggested.

 > addingFwdAddrsHandler_fwd_only_add_Ok: !''Ok!'' could start with the
 small letter.
 >

 OK. :)

 > Instead of this:
 > {{{
 >     EXPECT_TRUE(!update_msg);
 > }}}
 >
 > you could simply do:
 > {{{
 >     EXPECT_FALSE(update_msg);
 > }}}
 > In fact, you should even use ASSERT_FALSE, because the further part of
 the test relies on this.
 >

 Fixed.

 > Question: in this statement:
 > {{{
 >             // Subsequent passes should reuse the request.
 >             EXPECT_TRUE(prev_msg == curr_msg);
 > }}}
 > do you intend to compare pointers to check that they point to the same
 object, or you intend to check that the messages look the same but they
 can be distinct pointers? I guess, if one of them gets copy constructed
 they may be different pointers. But I don't know what the intent was.
 Perhaps the extending the commentary would be a good idea.

 Added explanatory text here.  The idea is that only upon initial entry
 into a
 state which builds requests, should it construct the request. Retries
 should
 resuse the same request.  This is controlled by doOnEntry() test at the
 top of
 such states, see addingFwdAddrsHandler.  Once we have created the request,
 the pointer value should not change on retries.  This comparison verifies
 the
 overall behavior.

 >
 > The same refers to this test: replacingRevPtrsHandler_time_out.
 >
 > selectingRevServerHandler: suggest that the list of server is extended
 to at least two, to verify that the transaction doesn't always transition
 to the ERROR state when IO error occurs, but it rather loops until a list
 get exhausted.
 >

 Extended as requested.


 > Instead of this:
 > {{{
 >     EXPECT_TRUE(!update_msg);
 > }}}
 >
 > you could simply do:
 > {{{
 >     EXPECT_FALSE(update_msg);
 > }}}
 > In fact, you should even use ASSERT_FALSE, because the further part of
 the test relies on this.
 >
 >

 Fixed this.
 >
 > '''src/bin/d2/tests/nc_trans_unittests.cc'''
 > The new tests: dnsUpdateRequestAccessors and dnsUpdateResponseAccessors
 lack description.
 >

 I cleaned this up.


 > The description of the makeCannedTransaction is somewhat short. The
 !''canned!'' term is not generic enough for me. I don't understand why we
 have to create the !NameChangeRequest from Json, rather than using the
 constructor. Perhaps it is for the completeness of the test, which assumes
 that we are simulating reception of the NCR. But, that could be mentioned.
 Explanation of the !''star!'' sign would be helpful - see the construction
 of the DnsDomain object.
 >

 I added commentary.


 > dnsUpdateRequestAccessors: The comment seems to be truncated or invalid.
 > {{{
 >     // Verify that the DNS update request accessors.
 > }}}
 >
 > This test is inconsistent with respect to use of asserts and expects.
 Take a look at this:
 > {{{
 >     // Post set, we should be able to fetch it.
 >     EXPECT_TRUE(name_change->getDnsUpdateRequest());
 >
 >     // Should be able to clear it.
 >     ASSERT_NO_THROW(name_change->clearDnsUpdateRequest());
 >
 >     // Should be empty again.
 >     EXPECT_FALSE(name_change->getDnsUpdateRequest());
 > }}}
 >
 > If the first statement fails (so as the update request request is false)
 the test will continue. Assume that the second statement doesn't throw an
 exception, the third statement passes because update request has been
 false since or prior to the first statement. However, the fact that the
 third statement passes doesn't really mean that the cleanDnsUpdateRequest
 worked, because in our case the state of the update request hasn't changed
 anyway.
 >
 > On the other hand, if there is something wrong, the function will fail
 sooner or later, giving the indication that something was wrong. So, it is
 not the big deal, I have to admit. But, it just a suggestion to have a
 look into asserts vs expects and possibly there is some room for
 improvement.
 >

 Sorry, this was just sloppy work.
 I cleaned this test up a bit, accounting for your comments. Did the same
 for
 same to be true with dnsUpdateResponseAccessors.

 > Neither sendUpdate, retryTransition nor setUpdateAttempts are unit
 tested. I understand that it may be more involved to test sendUpdate so
 perhaps the reference should be added to unit tests that actually test its
 behaviour (in nc_add_unittests?) to say that it is unit tested there. In
 any case it would be the most appropriate to test sendUpdate in this file.
 >
 >

 There are unit tests for all of these now.  The sendUpdate unit tests
 required
 a fair amount of effort but it is a good thing you commented on it.  It
 uncovered a few things that I have fixed.

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