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-20121106
  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:16 vorner]:

 > > 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.
 >
 > What I mean is, the code is needlessly complicated even when the benefit
 is
 > small. I don't want to push it strongly, just pointing it out it looks
 > complicated.

 I really don't understand it if you mean this one is "extreme" or
 complicated:
 {{{#!cpp
         {
             typename MutexType::Locker locker(queue_mutex_);
             command_queue_.push_back(
                 datasrc_clientmgr_internal::Command(command, arg));
         }
         cond_.signal();
 }}}
 while this is not:
 {{{#!cpp
         {
             typename MutexType::Locker locker(queue_mutex_);
             command_queue_.push_back(
                 datasrc_clientmgr_internal::Command(command, arg));
             cond_.signal();
         }
 }}}

 I don't mind changing the former to the latter, but since I may simply
 misunderstand you, I still didn't change that.

 > > > 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.
 >
 > Well, you can make both push and shift (`pop_front`) be amortized O(1).
 I'm not
 > sure if stl does that, though. But then, there's no need to repeatedly
 pop
 > them. It could look like this (symbolically):
 > {{{#!c++
 > for (ever) {
 >       vector<Command> commands;
 >       condition.wait();
 >       // Empties queue and gets the commands locally
 >       queue_.swap(commands);
 >       // No need to pop here or empty the vector, we'll create a new
 empty one.
 >       FOREACH(const Command& command, commands) {
 >               handleCommand;
 >       }
 > }
 > }}}

 Ah, okay, you're right in that sense.  For now, however, I keep the
 current code because efficiency in queue operation isn't a big issue
 here anyway, and because we may want the pop behavior in future
 extensions.

 > > 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 think it would be better to put abort in there at least for now, and
 have
 > some better error handling deferred for a later ticket (but I don't know
 what
 > better should be done).

 Okay, changed.  I'll create a separate topic for this issue.

 > > 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.
 >
 > My understanding is, the pointers are swapped to point to the other
 target. But
 > as the pointer is passed by value, the change to the new_lists is lost
 when the
 > function is left.

 Ah, okay, you're right.

 > So I think it should be either propagated out (by passing a reference to
 a
 > pointer, for example, and maybe also tested), or changed as you propose.

 Changed so it just "sets" to the new value.

 > > > 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?
 >
 > Well, originally, it was like this, I believe:
 > {{{#!c++
 > "{" // The bracket on a separate line
 > "\"IN\": …
 > }}}
 >
 > The other possibility is joining the bracket there, so having this:
 > {{{#!c++
 > "{\"IN\": …
 > }}}
 >
 > I see these occurrences:
 > {{{#!c++
 >     ConstElementPtr reconfigure_arg = Element::fromJSON(
 >         "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
 >         "              \"cache-enable\": true}],"
 >         "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
 >         "              \"cache-enable\": true}]}");
 >     mgr.reconfigure(reconfigure_arg);
 > }}}
 >
 > {{{#!c++
 >     reconfigure_arg = Element::fromJSON(
 >         "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
 >         "              \"cache-enable\": true}]}");
 >     mgr.reconfigure(reconfigure_arg);
 > }}}
 >
 > {{{#!c++
 >     // A valid reconfigure argument
 >     ConstElementPtr reconfigure_arg = Element::fromJSON(
 >         "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
 >         "              \"cache-enable\": true}]}");
 >
 >     // On reconfigure(), it just send the RECONFIGURE command to the
 builder
 > }}}
 >
 > They are in tests `DataSrcClientsMgrTest::reconfigure` and
 `DataSrcClientsMgrTest::holder`.

 Okay, thanks.  Fixed them at least so that they are not obviously
 awkward.

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