BIND 10 #1976: use meta-or-container-of data source in b10-auth

[BIND 10 Development]

#1976: use meta-or-container-of data source in b10-auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120731
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  8
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hello

 Replying to [comment:13 muks]:
 > * Have you addressed the test suggestions in the earlier review?

 Yes, they are in the respective branch (trac1976). I'll need to merge all
 the branches together when finally merging.

 > * In `auth_srv.cc`, `90593cc3f05fbd272f11fc57c876487f9dbc3813` seems to
 leave a comment:
 > {{{
 >      /// In-memory data source.  Currently class IN only for simplicity.
 > }}}

 Not only a comment. The whole hot-spot cache was not used, so I removed
 it.

 > * Can the `cache-enable` key be renamed to simply `cache` as it's
 boolean? `cache-zones` can remain as-is.

 It could, but I don't think it is a good idea. I'd expect the entry
 `cache` to be a dict and contain all configuration of the cache, not only
 if it is enabled or not. For that reason, I'd like to keep it as it is, if
 you wouldn't mind.

 > * In `ConfigurableClientList::configure()`, what is the purpose of
 `allow_cache` argument? It cross checks with `dconf->get("cache-enable")`,
 but why can't it simply use that key instead?

 These are on completely different levels. With the `cache-enable`
 configuration option, the user might express the desire to have the data
 served from memory cache for performance, in auth.

 However, there are other modules. Once the XfrIn (for example) uses the
 client list, it would be a bad idea to cache the zone to memory. So auth
 sets allow_cache to true and XfrIn to false and user never sees the value
 of it.

 > * `ConfigurableClientList::configure()` can throw `isc::Unexpected` too
 (should be added in the doxygen doc)

 It is documented:
 {{{#!c++
     /// \throw Unexpected if something misbehaves (like the data source
     ///     returning NULL iterator).
 }}}

 > * In `ListTest.reloadNoSuchZone`, is the comment correct?
 > {{{
 > +    list_->configure(config_elem_zones_, true);
 > +    Name name("example.org");
 > +    // We put the cache in even when not enabled. This won't confuse
 the thing.
 > +    prepareCache(0, Name("example.com"));
 > }}}

 Yes, it is. But I updated it to explain what I mean better.

 > * In `ListTest.reloadMasterFile`, currently the code modifies the zone
 in memory and reloads the master file to see if it was reset to the old
 data. Can you add a test that modifies the master file instead and checks
 if it shows up after the reload?

 Such test would be possible, but not as simple. Would it test anything
 extra to what we test now?

 > * You may simply want to fold `validate()` into
 `LoadZoneCommand::exec()`

 Yes, you're right, it got simplified a lot during the changes.

 > * I have pushed these patches:

 Thanks for them.

 Changes are pushed in the trac1976-cont-3 branch.

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