BIND 10 #2210: support "reconfigure" command in the configurator thread

[BIND 10 Development]

#2210: support "reconfigure" command in the configurator thread
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121023
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I have one substantial point to discuss.  Others are minor.

 '''substantial'''

 - `doReconfigure()` in datasrc_clients.mgr: I think it's safer to
   define new_clients_map outside of the try block (more essentially
   the scope for the locker):
 {{{#!cpp
             isc::datasrc::DataSrcClientListsPtr new_clients_map;
             try {
                 new_clients_map = configureDataSource(config);
 }}}
   to make it more clear that swapped map (stored in new_clients_map
   after swap) will be released outside of the lock.  That's probably
   already the case in the current code due to the ordering of the
   variable definition, but I'm afraid it's subtle and possibly
   fragile.  We also probably need to comment about the rationale of
   the lock timing, just like we did in main.c:datasrcConfigHandler().

 '''general'''

 - I made some minor style fixes.

 '''auth_messages.mes'''

 - don't we need to be more specific like clarifying it's about data
   source configuration in the short description?
 {{{
 % AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR Error in
 configuration: %1
 }}}
   Or would that be redundant because it should be pretty obvious from
   the message ID?  Same for the other two messages.

 - Is "from before the reconfigure command" grammatically correct?  (I
   can understand what it means though)
 {{{
 an error. The system is still running with the data sources from before
 the reconfigure command, and the configuration data needs to be checked.
 }}}

 '''datasrc_clients_mgr.h'''

 - perhaps a matter of taste, but you can generally omit "isc::" in
   namespace specifications as it's already in the isc namespace, e.g.:
 {{{#!cpp
     isc::datasrc::DataSrcClientListsPtr clients_map_;
 }}}
   at least this was intentionally omitted (for brevity) in other part
   of this file.

 - regarding the TODO, I think this can be a brief one.  and in any
   case I'd fold the line (it's currently long)
 {{{#!cpp
     RECONFIGURE,  ///< Reconfigure the datasource client lists,
 configuration argument (TODO: describe here?)
 }}}

 - maybe some short description here?  at least clarify map_mutex_ is
   to protect clients_map_.
 {{{#!cpp
     isc::datasrc::DataSrcClientListsPtr clients_map_;
     MutexType map_mutex_;
 }}}

 - I guess it's probably better to create an empty map in the
   `DataSrcClientsMgrBase` constructor, instead of starting from a NULL
   pointer.  That way we won't have to worry about the case it's a NULL
   every time we use it.  (Note that this will require changes to some
   tests).

 - `doReconfigure(): related to the previous point, we might want to
   check new_client_map isn't NULL.  This should be ensured by
   configureDataSource() which also belongs to auth, and in that sense
   we could even assert() it.  throwing on NULL would also be okay
   though.

 - in `doReconfigure()`, you can use shared_ptr::swap:
 {{{#!cpp
                 //std::swap(new_clients_map, *clients_map_);
                 new_clients_map.swap(*clients_map_);
 }}}
   (in this particular case there wouldn't be much difference in
   effect, though).

 - is `data::TypeError` the only possible error from the datasrc layer
   (propagated via configureDataSource) for configuration error?
             } catch (const isc::data::TypeError& type_error) {
                 LOG_ERROR(auth_logger,
 AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR).
                     arg(type_error.what());

 - full data source reconfiguration is a big event, so I think it
   should be logged for non error cases too, maybe both before
   and after, at the INFO level.

 '''datasrc/client_list.h'''

 Just a comment: I don't object to defining `DataSrcClientListsPtr`
 here instead of `AuthSrv`.  At least the concept isn't specific to the
 auth server implementation, but I'm also not sure if it's of general
 interest.  We could consider many kinds of variations like this, such
 as a set of client lists, maps from strings to lists, etc, depending
 on the purpose of the application.  Clearly we wouldn't like to define
 a shortcut typedef for each and every one of such things.

 Also, I'm afraid `DataSrcClientListsPtr` might not really be a good
 name (disclaimer: it was me who named it), especially if it's defined
 in a more public space.  It doesn't clarify it's a (pointer to) map,
 and the sense of 's' in 'Lists' could be easily overlooked.  I
 actually didn't like that name, but I couldn't come up with a clearer
 but not too verbose one (even this one is quite long, and
 `DataSrcClientListMapPtr` sounded awful).  So, if you have a better
 idea of naming it, please also make that change at this opportunity.

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