BIND 10 #2204: revise auth DataSourceConfiguratorGeneric::reconfigure()

[BIND 10 Development]

#2204: revise auth DataSourceConfiguratorGeneric::reconfigure()
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 I noticed the new approach is simpler. However, it drops one place for
 future
 improvement. I planned that when there's an update, we would look through
 the
 old list and reuse the clients with the same configuration, instead of
 creating
 them again. I don't think this was implemented, but with your new
 approach, it
 doesn't seem to be easily possible. If we want to provide the shared-
 memory
 approach soon, I'm willing to trade the thing for simpler code, but we
 should
 at least consider if it is OK.

 Also, I thought that when there's an update of configuration and the top-
 level
 item is unchanged, it is not contained in the update. That is, if we added
 another item into data_sources besides classes and the new one got
 changed, we
 would get an update not containing any classes element. This seems not to
 be
 taken into account in the handler in main.cc.

 This comment seems to be wrong, strictly speaking, the return value is not
 really ignored, it is stored into a variable (which is then not used).
 It's not
 that'd require full formalism, but it is confusing ‒ I was trying to see
 which
 return value is ignored.

 {{{
 By ignoring the return value we let the
 old lists be destroyed.  Lock will be released immediately after the
 swap.
 }}}

 This comment looks like it misses the third form, should it be „returns“?
 Or
 „will return“?

 {{{#!c++
 /// This will hook into the data_sources module configuration and it
 return
 /// a new set (in the form of a shared pointer to map) of data source
 client
 /// lists corresponding to the configuration.
 }}}

 The tests still speak about rollback, but currently, there's no rollback
 in the
 code. It might be misleading, so I think the names of tests and comments
 should
 be updated:

 {{{#!c++
 // Check that we can rollback an addition if something else fails
 }}}

 {{{#!c++
 // Check that we can rollback a deletion if something else fails
 }}}

 {{{#!c++
 // Check that we can roll back configuration change if something
 // fails later on.
 }}}

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