BIND 10 #2213: revise LoadZoneCommand::exec() of b10-auth to use the configurator thread

[BIND 10 Development]

#2213: revise LoadZoneCommand::exec() of b10-auth to use the configurator 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:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:12 jinmei]:
 > > We could add support for that (by factoring out the first part of the
 real threaded handler, that looks up the zone in the map), but it would
 mean doing that twice too (however, this may be relatively cheap anyway).
 > >
 > > Shall we do that too? (it should make error more reporting complete,
 up to the actual reading of data at least)
 >
 > I have a mixed opinion about this.  In terms of user-friendliness it's
 > better to do double check; mistyping a zone name wouldn't be uncommon,
 > and the overhead of the double check shouldn't be a big concern in
 > this case.  On the other hand, redundant behavior is generally not
 > good, and, at least in theory, the check at the manager isn't 100%
 > reliable anyway; the builder may be updating the data source clients
 > at the time the manager receives the reload command, and the specified
 > zone may disappear when the builder finally receives the command from
 > the manager (although this scenario is more unlikely than a user
 > mistyping the zone name).
 >
 > So, I'd leave it to you.  But if you decide to add the check at the
 > manager, I suggest combining the check code some way rather than
 > copying the same logic from the builder to the manager.
 >

 Yes we'd have to factor the check itself out a bit, and lock the map
 (which itself needs careful checking). I'll leave it for now, consider it
 a bit more, and if I still want it, I'll create a new ticket for just
 that.

 >
 > - you can skip explicitly adding else...if here:
 > {{{#!cpp
 >         } else {
 >             // Also check if it really is a valid name
 >             try {
 >                 dns::Name(args->get("origin")->stringValue());
 >             } catch (const isc::Exception& exc) {
 >                 isc_throw(CommandError, "bad origin: " << exc.what());
 >             }
 >         }
 > }}}

 done

 >
 > - this doesn't parse:
 > {{{#!cpp
 >     // but atm the config/command API does not allow that to be done
 >     // easily.
 > }}}
 >   maybe atm = "at the moment"?  If so it parses, although I think it's
 >   first time to me to see this acronym.
 >

 Yes, atm = "at the moment", I use it so often I don't even realize it
 anymore :) expanded the acronym.

 > '''Others'''
 >
 > - I suggest removing AUTH_LOAD_ZONE from the log message.

 done

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