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

[BIND 10 Development]

#3087: Develop NameAddTransaction class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:  Task# 4.2     |  Sprint-DHCP-20131204
           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: 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.

 The review follows:

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

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

 Now, a few typos in the individual messages...

 - DHCP_DDNS_FORWARD_ADD_REJECTED: typo !''itwas!''

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

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

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

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

 - DHCP_DDNS_REVERSE_REPLACE_IO_ERROR: Why last line wrap?

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

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

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

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

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

 readHandler: The brief description seems to be truncated.

 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_

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

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

 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?

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

 NameAddTransactionTest::prepHandlerTest lacks description.

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

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

 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.

 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.

 }}}

 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.

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

 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.

 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.

 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.

 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.



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

 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.

 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.

 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.

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