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

Comment (by jinmei):

 Replying to [comment:10 vorner]:

 Thanks for the review.

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

 Yeah, I was actually aware of that, and I admit I should mention it.
 At least for our middle term goals (like for the next beta release) I
 don't see any benefit to keep complicated interface with possible
 future extensions.  But if you want to leave some consideration notes
 on this point, I suggest adding the following to the detailed
 description for configureDataSourceGeneric():

 {{{#!cpp
 /// \note In future we may want to make the reconfiguration more efficient
 /// by only creating newly configured data and just moving the rest from
 /// the running configuration if they are used in the new configuration
 /// without any parameter change.  We could probably do it by passing
 /// the old lists in addition to the new config, but further details are
 /// still to be defined yet.  It will surely require changes in the
 /// data source library, too.  So, right now, we don't introduce the
 /// possibility in the function interface.  If and when we decide to
 introduce
 /// the optimization, we'll extend the interface.
 }}}

 I personally don't think the current approach makes this goal
 particularly more difficult, though (I'm not saying it's easy - it was
 already uneasy task, and I don't think the current change does not
 make it even harder).  I can think of some possibility of doing it by
 extending the current interface, although there can be unseen pitfalls
 in details.

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

 Sorry, I don't understand this.  Could you some specific example?

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

 Ah, sorry, it didn't make sense.  I guess it was an intermediate
 version when the return value was indeed "ignored".  I've updated the
 comments so it will match the code.

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

 That should have been "will return".  Fixed.

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

 Good catch, I think we only have to change the wording of test name
 and comments.  I've updated it that way.

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