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 4c74dff22bd0cd130120d98847c16152ddf39bc6

 Replying to [comment:13 tmark]:
 > Replying to [comment:12 marcin]:
 > > > > '''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.
 >
 > >
 > > New doxygen description has misspellings.
 >
 > Fixed.

 I still find that weird: !''This handler, therefore, '''as the''' is the
 entry point ...!''.

 > > > >
 > > > > '''src/bin/d2/tests/nc_add_unittests.cc'''
 > > > >
 >
 > > Suggestion: for the tests containing a loop with multiple servers, it
 is convenient from the debugging standpoint if the error message contains
 the server address. So for example:
 > > {{{
 > >     for (int i = 0; i < num_servers; i++) {
 > >         DnsServerInfoPtr curr_server = name_add->getCurrentServer();
 > >         // Run selectingRevServerHandler.
 > >         EXPECT_NO_THROW(name_add->selectingRevServerHandler())
 > >             << "failed to select server " <<
 curr_server.getIpAddress().toText();
 > > ...
 > >
 > > }
 > > }}}
 > > It is not a must for this ticket. Just something for consideration.
 > >
 >
 > Given the possibility that curr_server might be an empty pointer, I
 modified
 > the tests to output the number of servers and number of selections at
 the point of failure:
 >
 > {{{
 >         ASSERT_NO_THROW(name_add->selectingRevServerHandler())
 >               << " num_servers: " << num_servers << " selections: " <<
 i;
 > }}}
 >
 > I also changed some EXPECTs to ASSERTs.

 That's fine, but that could have been done for other asserts in the loop
 too.

 Please fix the typo and if you concur with the second comment, please do
 appropriate changes. I don't need to see this ticket again.

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