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

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:12 vorner]:

 > 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();
 >     }
 > }}}

 I don't think the current code is "extreme" or what's wrong with it.
 But since it's not a performance sensitive path, if you strongly want
 that I don't mind putting signal() inside the block either.  Right now
 I didn't touch the code.

 > 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_);
 > }}}

 Ah, right, thanks for pointing it out.  I replaced it with swap().

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

 Since we use it as a FIFO queue, a vector will make either push or pop
 slower, so for now I didn't change that.  But as you also pointed out,
 such level of performance doesn't matter in this case, so I don't mind
 changing it to vector.

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

 Yes, I was aware of that.  My original thought (which should have been
 mentioned in this ticket) is to handle it in a later ticket.  But I'm
 also okay with aborting the program (from the thread) when an
 unexpected exception happens in the thread in preferring explicit
 failure than leaving unworkable state for some unpredictable period.

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

 It might even be a subject of discussion whether we want this, but I'm
 okay with creating a ticket(s) for this.  In any case it's quite
 separate from the topic of this ticket, so I'll do it after the
 completion of this task.

 > 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 only reason is that `AuthSrvTest` wouldn't work otherwise.  I've
 added some notes on this, and created a ticket so we can eventually
 clean this up (#2395).

 > 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`?

 Hmm, maybe I misunderstand it but I believe this method should
 actually swap the contents.  That said, the expected usage (some tests
 and query_bench) actually doesn't need the "swap" behavior anyway;
 they only need to install the specified set of zone data for tests or
 benchmarking.  So, I'm okay with changing the name to, e.g.,
 `setDataSrcClientLists()` or `installDataSrcClientLists()` and just
 overriding clients_map_:

 {{{#!cpp
     void setDataSrcClientLists(datasrc::DataSrcClientListsPtr new_lists) {
         typename MutexType::Locker locker(map_mutex_);
         clients_map_ = new_lists;
     }
 }}}

 if you like it better.  Right now I didn't touch the code for this.

 > 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}]}");
 > }}}

 Hmm, I see this one is ill-formatted, but I'm not sure what exactly
 you suggested by "either...or...".  I also couldn't find other ill
 cases by 'grep MasterFiles *.cc' for tests.  Could you be more
 specific about your suggestion (or feel free to change it/them to what
 you want like to be) and specify other cases you think need to be
 fixed?

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