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:14 jinmei]:
 > Replying to [comment:12 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.

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

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

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

 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.

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

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