BIND 10 #3059: Create initial UpdateMgr class

[BIND 10 Development]

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

 Done.


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

 Done.

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

 I did not make them private.  I made them protected and derived a test
 class
 from it.

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

 I agree that we should make improvements when we find the need, because
 often we never get another chance. I looked at returning an Assertion
 result, and even with that one must still write something like the
 following:

 {{{
 ASSERT_TRUE(checkServer(...))
 }}}

 to get the failure detail desired.  This doesn't seem to be any real
 improvement.  Under GoogleTest 1.6, you get better output from this but we
 have to be compatible with as far back as GoogleTest 1.4.  I'm not sure
 you can achieve the detail granularity I want with less code and have it
 compile under older versions of GoogleTest.  I am getting the behavior I
 want from this code and am going to leave it alone.

 >
 > 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).!'' ?

 Yes, you're right. How's this:

 {{{
 6xx. [func] tmark

 Added D2UpdateMgr class to DHCP_DDNS. 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).
 (Trac #3059 git TBD)
 }}}

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