BIND 10 #2219: switch to memory-efficient version of in memory data source

[BIND 10 Development]

#2219: switch to memory-efficient version of in memory data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120925
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:8 vorner]:
 > Lettuce tests fail here. I'm not sure it is related, but it might be:

 That's an NSEC3 test, right?  It's due to bugs in #2218 and will pass
 once we merge both #2218 and #2219 (that's one of the failures I
 mentioned in the "Final note" of my request-for-review comment).

 > Regarding the hack with configuration for the first time, this seems to
 be fragile. What if someone fixed the cause of the problem? Or if someone
 tries to use the class properly? Wouldn't it be better to remember the
 last configuration and not reconfigure in case it is the same?

 I know it's an ugly hack, but I don't like to tweak the code further
 within this branch.  If you don't like the current version because
 it's susceptible to future changes, I propose either:
 - just skip this hack for now.  it'll still work correctly while initial
 start up time can be doubled
 - update the "HACK" comment in main() like this:
 {{{#!cpp
         // HACK: The default is not passed to the handler. This one will
         // get the default (or, current value). Further updates will work
         // the usual way.
         // TODO: remove another hack in
         // DataSourceConfiguratorGeneric::reconfigureInternal() when we
 solve
         // this problem and stop this explicit reconfiguration.
 }}}
   so it'll be more likely to remember the necessary update.

 > I'm not sure about the meaning of this comment. Could you explain it
 little bit more, what is happening there?
 >
 > {{{
 >         // If and when we allow non absolute label at the origin (e.g.
 for
 >         // the convenience of out-of-zone glue handling), we first need
 to
 >         // construct the absolute label sequence and then construct the
 name.
 > }}}

 How about this:
 {{{#!cpp
         // In future we may allow adding out-of-zone names in the zone
 tree.
         // For example, to hold out-of-zone NS names so we can establish a
         // shortcut link to them as an optimization.  If and when that
 happens
         // the origin node may not have an absolute label (consider the
 zone
         // is example.org and we add ns.noexample.org).  In that case
         // we first need to construct the absolute label sequence and then
         // construct the name.
 }}}

 You may wonder if this is a premature setup, though.  If you think so,
 an alternative would be to just assert the labels be absolute:
 {{{#!cpp
     const LabelSequence node_labels =
 zone_data_.getOriginNode()->getLabels();
     assert(node_labels.isAbsolute());
     data = node_labels.getData(&data_len);
     ...
 }}}
 (and remove the added unit test)

 I'm okay with either way.

 > Also, I noticed some indentation that probably does not adhere to any
 style guide I've ever seen O:-).
 > {{{#!diff
 > -    return (ZoneFinderResultContext(code, createTreeNodeRRset(node,
 rrset,
 > -                                                              rrclass,
 rename),
 > +    return (ZoneFinderResultContext(code, createTreeNodeRRset(
 > +                                        node, rdset, rrclass, options,
 rename),
 >                                      flags, node));
 > }}}

 Yeah that's a kind of "non standard" exception to keep lines shorter.
 There are already some similar cases like this though.  If this looks
 too awkward, an alternative would be:
 {{{#!cpp
     return (ZoneFinderResultContext(code, createTreeNodeRRset(node, rdset,
                                                               rrclass,
 options,
                                                               rename),
                                     flags, node));
 }}}

 > Besides that, I think the branch was rather longer than optimal, and
 there were many small and not really related changes. I'm not sure I
 didn't overlook anything (and I hope this sentence is correct in English,
 there seem to be too many negatives).

 I know this branch contains too many small things (and some of them
 were completely unrelated like pure style fixes).  And, in fact,
 that's why I decided to skip one other update: migrate the static data
 source implementation.  I wish I could have separated these, but it's
 pretty difficult to predict how many things we need to adjust in an
 integration task.

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