BIND 10 #3059: Create initial UpdateMgr class

[BIND 10 Development]

#3059: Create initial UpdateMgr class
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130731
           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:

 Replying to [comment:6 tmark]:
 > Replying to [comment:5 marcin]:
 > > '''src/bin/d2/d2_config.cc'''
 > > matchDomain: Why do you have to reset the domain pointer on the
 function entry? I suggest that the pointer provided by the caller is not
 modified if there is no match. I would understand that the pointer is
 reset if there was no boolean value return which signals that the match
 was found. You could choose to signal no match with NULL pointer. Since,
 you have the return value there is no need to modify the pointer, allowing
 the caller to use it further if there was no match.

 This hasn't been addressed. Please let me know what you think.

 > >
 > > Instead of using raw (unsafe) strings and memcmp you could use
 std::string objects and use equality operator and .compare function like
 this:
 > Done.

 Actually, it could be further simplified by removing the req_name and use
 fqdn instead. I committed the change to the branch. Please pull the
 change.

 > >
 > > Are there any functions in UpdateMgr that should be private? For
 example, I can see that the checkFinishedTransactions is called internally
 by sweep(). There is a question if it will be also called externally? Same
 for pickNextJob().

 This hasn't been addressed.

 > >
 > > '''src/bin/d2/tests/d2_cfg_mgr_unittests.cc'''
 > > checkServer: Why not declare checkServer as void and perform
 !''expects!'' there like this:
 > >
 > > {{{
 > >     EXPECT_TRUE(server);
 > >
 > >     // Check hostname.
 > >     EXPECT_EQ(hostname, server->getHostname());
 > > ...
 > > }}}
 > > etc. ?
 >
 > Because the  test log output is less precise that way.  If I do as you
 suggest
 > the log loosk about like this:
 >
 > {{{
 > [ RUN      ] DdnsDomainTest.DdnsDomainListParsing
 > d2_cfg_mgr_unittests.cc:100: Failure
 > Value of: server->getPort()
 >   Actual: 500
 > Expected: port
 > Which is: 700
 > [  FAILED  ] DdnsDomainTest.DdnsDomainListParsing (0 ms)
 > }}}
 >
 > The way it is written now, the log looks like this on a failure:
 >
 > {{{
 > d2_cfg_mgr_unittests.cc:100: Failure
 > Value of: server->getPort()
 >   Actual: 500
 > Expected: port
 > Which is: 700
 > d2_cfg_mgr_unittests.cc:827: Failure
 > Value of: checkServer(server, "", "127.0.0.5", 700)
 >   Actual: false
 > Expected: true
 > }}}
 >
 > Notice that you can easily see where it failed.  This is helpful when
 there are many
 > calls to the same method in many tests.  I actually could make it return
 an Assertion result
 > which is a technique I only recently learned.  However, at this point in
 the schedule there are more pressing things to do and these tests all
 function correctly.

 Fair enough. One comment regarding !''However, at this point in the
 schedule there are more pressing things to do and these tests all function
 correctly.!'' I am sure there will never be a good time to fix things like
 this. In other words, if you're planning to change this to
 !''!AssertionFailure approach!'' I suggest that it is rather done today.
 If you  don't do it today, I bet it will remain like this forever. There
 is a general practice among all developers to say !''we will fix it
 later!'' and 90% of these things are never fixed.

 In general. I now understand the motivation and I will not insist that you
 change it.


 Regarding the !ChangeLog. Shouldn't it be: !''This class '''is''' the
 DHCP_DDNS task master,
 instantiating and supervising transactions that carry out the DNS updates
 needed to fulfill the requests (!NameChangeRequests) received from
 DHCP_DDNS
 clients (e.g. DHCP servers).!'' ?

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