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

 On reviewing the revised version, I guess we need a bit more
 discussion on catching exceptions and logging the events in
 doReconfigure() (see the end of this comment for specific
 suggestions).  Other changes look okay.

 I now guess it's not to good all general std::exception or "..." at
 this level.  If we see something other than those derived from
 `isc::Exception`, it's basically a fatal one like bad_alloc or
 something that's not really expected and we don't know how to recover
 from.  So rather than pretending nothing has happened except for
 logging the event, I think the right reaction in this case is to let
 it go through to the top level and (as a result) terminate the thread.

 Instead, we catch `isc::Exception` at the highest level of this method
 and log it.  And, to this end, I'm not sure if we explicitly need to
 catch `TypeError`.  If it generally only happens due to an internal
 bug, I guess we can reasonable include such cases in the generic
 `isc::Exception` case.

 And, about the detailed log message for the general error case
 (AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_ERROR):
 {{{
 reconfigure, but raised an exception while reconfiguring. This is most
 likely a bug in the data source, but it is probably a good idea to verify
 the configuration itself. The system is still running with the data
 sources
 }}}
 actually, I suspect the "most likely" cause in this case is a syntax
 or semantics error in the zone contents.  And, some less likely, but
 probably more likely than internal bugs, cause would be other generic
 data source errors such as missing .sqlite3 file or (when supported)
 connection failure to the DB server.

 The latter case generally seems to result in `DataSourceError` (thrown
 via the `DataSourceClientContainer` wrapper).  I guess we should also
 make sure that exceptions from the in-memory loader due to zone file
 errors are inherited from `DataSourceError` so we can unify the
 exception for these cases.

 And, about this:

 > I *think* it is correct, but it may be a tiny bit unclear; how does this
 sound:
 > {{{
 > The system is still running with the data sources that were
 > previously configured (i.e. as if the configuration has not changed),
 > but the configuration data needs to be checked.
 > }}}
 >
 > (I now wonder if I should also add that if this is not fixed, upon a
 restart, the system would revert to defaults)

 The revised text looks clear.  But, I suspect what happens on restart
 is different than reverting to defaults - I suspect in that case the
 server will be running with an empty client-list map (in effect,
 returning REFUSED to any queries)...hmm, in that sense "default" is
 not necessarily incorrect, but it's the `AuthSrv's` implementation
 default, but not the configuration default.

 To summarize, my specific suggestion is:

 - In doReconfigure(), catch `ConfigurationError`, `DataSourceError`,
   and `isc::Exception` (and let others be propagated).
 - For the log description `isc::Exception`, clarify that it's most
   likely a low level errors within the data source (broken zone file
   or connection failure to the server), but can also mean internal
   bugs.
 - (out of scope of this ticket) Create a ticket to make sure zone file
   errors are derived from `DataSourceError`.  It will make it clearer
   that `isc::Exception` is most likely an internal bug.

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