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        |  closed
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:  Task# 4.2     |  Sprint-DHCP-20131204
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |  complete
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):

 * status:  reviewing => closed
 * resolution:   => complete


Comment:

 Replying to [comment:14 marcin]:
 >
 > 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 ...!''.
 >

 You keep finding it because I never saw it to begin with but it is fixed
 now.


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

 Added the additional output to ASSERTs in the loop.

 Merged with git 8f99da735a9f39d514c40d0a295f751dc8edfbcd

 Created ChangeLog entry 801.

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