BIND 10 #2211: update the data source reconfigure command so it uses thread

[BIND 10 Development]

#2211: update the data source reconfigure command so it uses 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:  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

 Replying to [comment:9 jinmei]:
 > It depends on #2210, which is now under review, but #2210 seems to be
 > mostly ready for merge so it should be safe to start reviewing this
 > branch.

 While reviewing this branch, I noticed few problems that do not come from
 this
 branch directly (I needed to read a little bit of surrounding code to
 understand). I suppose they are from #2210. I'll list them here, but if
 needed,
 I'll forward them or open a new ticket.

 This one, I understand the desire to keep the scope of lock as small as
 possible, but isn't this a bit to the extreme (note you can `.signal()` a
 condition with locked mutex)?
 {{{#!c++
     void sendCommand(datasrc_clientmgr_internal::CommandID command,
                      data::ConstElementPtr arg)
     {
         {
             typename MutexType::Locker locker(queue_mutex_);
             command_queue_.push_back(
                 datasrc_clientmgr_internal::Command(command, arg));
         }
         cond_.signal();
     }
 }}}

 Is there a need for `splice`? I think `swap` is simpler method that does
 what
 we want there.
 {{{#!c++
 current_commands.splice(current_commands.end(),
                         *command_queue_);
 }}}

 Also, with `swap`, we could use a vector instead of list, which might
 happen to
 be slightly faster. But I guess it isn't in a performance-sensitive place.

 The body of the thread may throw:
 {{{#!c++
     } catch (const std::exception& ex) {
         // We explicitly catch exceptions so we can log it as soon as
 possible.
         LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED).
             arg(ex.what());
         throw;
     } catch (...) {
         LOG_ERROR(auth_logger,
 AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED);
         throw;
     }
 }}}

 This has two problems. First, the `UncaughtException` mechanism is a
 safe-catch, not meant for a general use. As the documentation says, it
 should
 be avoided to throw from there.

 But the bigger problem I see is, if the thread terminates by an exception,
 the
 master thread does not know that until it `wait()`s for it. The main
 thread
 will just keep sending commands and the queue will grow and grow for ever.
 So
 I'd propose either inform the main thread somehow (but I have no idea how)
 or
 terminate the application if the exception happens (from inside the
 thread).

 > The real new code is small, up to around 0e70ad3.  Most of the other
 > changes are refactoring: updating exiting auth server's code so it
 > uses the new framework, adjusting test setup, cleanup deprecated
 > stuff.  I believe existing test semantics is (mostly if not all)
 > preserved, but some are moved to datasrc_clients_mgr_unittest and the
 > change may seem bigger due to that.

 The main problem for me was not so much the size as it is scattered around
 a
 lot of places. It took me a while to get a glimpse about what is
 happening. But
 we do need a challenge from time to time O:-).

 > I've not all of the additional things hinted in the tickets as the
 > branch was getting bigger: I've skipped updating `ModuleCCSession`
 > class, and kept the data source config handler still in main.cc.

 Do you want to put them to another ticket, or do it after the second round
 of
 review?

 '''Some more (mostly minor) comments''':

 Typo („thek“):
 {{{
 +    // Get access to data source client list through the holder and keep
 thek
 }}}

 Why does it return share pointer if it explicitly says the ownership is in
 the
 class? It is not intuitive ‒ if you get a shared pointer, you'd expect it
 is
 usable as long as you keep it, which is not the case here. If you return
 an
 ordinary pointer here, it might make the caller more careful and read the
 documentation.

 The part around `NULL` seems to be badly worded, it doesn't sound English.
 {{{
 +    /// This method simply passes the new configuration to the builder
 +    /// and immediately returns.  This method is basically exception free
 +    /// as long as the caller a non NULL value for \c config_arg; it
 doesn't
 +    /// validate the argument further.
 }}}

 s/patter/pattern/?
 {{{
 +// Commonly used patter of checking member variables shared between the
 +// manager and builder.
 }}}

 I'd expect a method called „swap“ to actually exchange the object with the
 parameter. But the `swapDataSrcClientLists` doesn't modify the parameter,
 it
 keeps the original pointer pointing to the original object. I know the
 functionality is not needed, but the name is counter-intuitive. Maybe
 something
 like `replaceDataSrcClientList`?

 There seems to be some relict of re-wrapping lines. The beginning of the
 string
 has a starting `{` in it's own segment of string, but on the same line. I
 think
 it should be either in the same segment (not ending-quotes, space,
 starting-quotes) or on separate lines. It occurs multiple times, like
 this:
 {{{#!c++
         "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
         "              \"cache-enable\": true}],"
         "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
         "              \"cache-enable\": true}]}");
 }}}

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