BIND 10 #1608: implement ZoneFinder::Context::getAdditional() for in memory data source

[BIND 10 Development]

#1608: implement ZoneFinder::Context::getAdditional() for in memory data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  high   |  Sprint-20120320
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  auth   |           Total Hours:  3
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:14 jelte]:

 Thanks for the review.

 First, this one:

 > - One of the lettuce tests fails too (nsec3_auth.feature scenario 5;
 fails on the now missing glue from wildcards). We can temporarily change
 that one as well, but we should give a high priority to fixing this

 Good catch, and actually it's not about the deferred wildcard case,
 but something I intended to include in this branch but forgot.  I
 fixed it with a corresponding unittest case (62a3bb1).

 > - The two mappings of DomainNode::FLAG_USER are defined in different
 scopes. This may not be a problem (and in fact, defining them each in the
 smallest context as possible can be argued for), but it does feel like an
 abstraction violation, and I wonder if we shouldn't make define them in
 the same place.

 I'm not sure if it addresses the concern of "abstract violation", but
 I agree we should define them in some concept of the "same set" so
 it's clear we define different flag values for different purposes and
 have no collisions.  I tried to address this point by introducing a
 specific namespace (only visible in the .cc file) and define both
 flags there.  Doe that address your points?

 > - Do we want to support additional data and glue that is defined outside
 of this zone, but available to the system (i.e. in a different zone, or
 maybe a different data source)? Esp. from a different data source would
 require more memory (and scary reverse dependencies), since that can't
 simply point to existing memory, but if we use a 'real' lookup for the
 actual additional data, but we'd get wildcard matching for 'free'. But
 even for just the one in-mem datasource, we'd also need some way to reset
 them for any zone (if a child zone gets loaded later), even if we don't do
 dynamic updating of zone data. The answer may be 'no' :) (and if yes, out
 of scope for this ticket). Oh, and if 'no'; should we warn or error or
 missing needed glue?

 Good point.  I'd first like to point out that these concerns are not
 specific to getAddtional(); it's basically a (possible) "shortcut" of
 what the generic auth::Query code did, and these points apply to the
 original code, too.  So the question is whether we want to return
 out-of-zone additional records from the auth server in general.  Maybe
 we should discuss this at a biweekly call and/or bind10-dev.  For the
 purpose of this branch, I added an explicit note that it only returns
 in-zone (+ glue) records in the doxygen comment.  Regarding warning,
 maybe we could do at the construction time and especially for missing
 in-zone glue (under a zone cut), that would be helpful because it's
 more likely an operational error (BIND 9 warns about it as a post-load
 check IIRC) - but I'd defer it to a separate ticket (or to the general
 "zone loader" work).

 > - This comment suggests we are doing things that are very very dirty:
 > {{{
 > This is public only because it needs to be used outside
 > // of the \c RBNodeRRset class, but conceptually this is a private type.
 > }}}
 > It's less ugly than the comment would suggest, but still :) (private for
 whom, for instance) Any specific reason to prefer this over an abstract
 base class?

 Yeah, I know it's dirty.  Actually, I could completely hide this thing
 by introducing an additional layer of pimpl to RBNodeRRset.  But for
 now I preferred convenience since we'll soon need to revisit the
 fundamental structure of this stuff (we cannot hold the full RRset
 objects in-memory for various reasons including memory footprint), and
 since this header file isn't expected to be used by anything else than
 the in-memory implementation and its tests (in fact, if it doesn't
 have to be tested directly, the whole definition could be hidden
 within .cc).  I tried to add some more notes that hopefully are helpful.

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