BIND 10 #2107: redefine in-memory zone data

[BIND 10 Development]

#2107: redefine in-memory zone data
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 muks]:

 > > > * Why is `findRdataSetOfType()` not a member of `RdataSet`?
 > >
 [...]
 > I see that you want to handle the NULL case too now. Regarding making it
 a static member function, I think it would still be good, and you can
 reduce its name to just `findOfType()`.

 Okay, I changed them to static member functions.  I even simplified
 the name further, to find().  It's pretty likely that these are the
 only "find" variants, and even if not we'll be able to differentiate
 them by the signatures.

 > > > * `DomainTree::setData()` doesn't free the old data now, as can be
 seen in `DomainTreeTest.setGetData`. [...]
 > >
 > > Actually, we (in this branch) have clearly switched to the model where
 > > the application (not `DomainTree`) has the ownership of the data, by
 > > enforcing it to pass a deleter to `DomainTree::destroy()`.  [...]

 > Okay, in this case can we change `setData()` then so that it returns the
 old pointer, similar to `ZoneData::setNSEC3Data()`? It'll be useful for
 code like this too (though in this case, `next` is updated after the
 `setData()` which is bad):
 > {{{
 >     rdataset_aaaa->next = node->setData(rdataset_aaaa);
 > }}}

 Done.  The test was revised, and by that I noticed there was a leak in
 the previous version of the test (in this branch).  The revised test
 should have fixed it.

 > * A copy of `getNext()` seems to have been left in as a comment:
 > {{{
 >      /// get the same result by directly calling get() on \c next, it
 would
 >      /// help encourage the use of more efficient usage if we provide an
 >      /// explicit accessor.
 > -    //const RdataSet* getNext() const { return (next.get()); }
 >      const RdataSet* getNext() const { return (next.get()); }
 > }}}

 Ah, right, thanks for catching that.  Removed.

 > * I think `WILDCARD_NODE` is a better naming (sounds better) than
 `WILD_NODE`. Can we change that flag name in `ZoneData`?

 I don't have a strong opinion, so changed it as suggested, and while
 talking about it:

 > * `WILD_NODE` seems to be untested and unused for now.

 I know, but I knew we'd need it soon (actually in #2108, which you
 seemed to pick up), and wanted to make sure the definitions wouldn't
 be scattered.  I'd consider it actually belongs to #2108 and should be
 tested there.  There's in fact not much to test for it within this
 branch.

 > * Is it better to make the defintion of `ZoneData::isNSEC3Signed()` to:
 > {{{
 >     bool isNSEC3Signed() const { return (isSigned() && nsec3_data_); }
 > }}}

 Right now, I don't think so.  As documented the concepts of "NSEC3
 signed" and "signed" are intentionally separated and independent from
 each other.  We may need to revisit the relationship as we actually
 use them, but for now, I'll keep that concept.

 > * Can it be further clarified how the tree can be modifed (the first
 sentence here in `NSEC3Data`'s doc). What does "specific method" mean?
 > {{{
 > /// Modifying the tree must be done by specific method; the application
 > /// cannot directly change the content of the tree in an arbitrary way.
 > }}}

 I've tried to provide some more doc.

 > * This is a trivial nitpick, but in `ZoneData::destroy()` is it better
 to destroy the NSEC3 tree first before the `ZoneData` tree?

 I don't mind, but I don't see the specific need for it.  At the moment
 I didn't change that.

 > * Should `\return` be added to some of these `ZoneData` API doc for
 methods?

 Perhaps, but on the other hand, I didn't want to make the doc
 redundant in trivial cases like a simple getter method and the return
 value is clarified in the brief description, at the risk of looking
 inconsistent.  I skipped to be redundant for such cases.  If you
 prefer strongly, I'm okay with adding "redundant" return entries, but
 at the moment I kept the doc as it is.

 > * I think it's better to put the const and non-const versions of
 `ZoneData::getNSEC3Data()` next to each other in `zone_data.h`.

 My intent was to groups of "getter type" methods and "modify type"
 ones separately, based on the observation that once created we almost
 always use getter methods repeatedly, and in a performance sensitive
 context.  While it maybe premature, I thought defining frequently used
 methods closely might be a bit helpful for performance.

 Anyway, I've clarified the intent of grouping (not mentioning the
 possible performance effect) by explicitly grouping them with doxygen
 markups.

 > * What is the main purpose of `ZoneDataTest.throwOnCreate` test? If it
 is to check that leaks don't occur, it should be renamed to say that.

 It's for exception safety in general, and in practice it's for
 checking there's no leak after exception.  I'm not sure if renaming it
 helps, but I changed the name to something that may be better, and
 added comments about the intent.

 >
 > * In `ZoneDataTest.getSetNSEC3Data`, should we also check for the
 following (enforce it?):
 > {{{
 >      EXPECT_EQ(nsec3_data, zone_data_->getNSEC3Data());
 > +    EXPECT_TRUE(zone_data_->isSigned());
 >      EXPECT_TRUE(zone_data_->isNSEC3Signed());
 > }}}

 isSigned() has their own tests, so I don't see the need for adding
 them here.

 > * I have pushed some more comment updates and some minor code style
 changes to the branch.

 Thanks, they look good, with one minor update (8052c97).

 > The rest of the stuff seems fine. :)

 Thanks.  Now, assuming the revised version should be mostly if not
 completely okay for merge, I've created a separate branch for merge at
 "trac2107merge" and pushed it.  Merging this branch will cause a non
 trivial set of conflicts, so I've committed conflict resolution in the
 "merge" branch.  If you think the main 2107 branch is okay, could you
 review the conflict resolution?  Then I'll merge the 2107merge branch
 instead of the original one.  (Also, for your work at #2108,
 trac2107merge should be a better base).

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