BIND 10 #1605: introduce special RRset for in memory data source

[BIND 10 Development]

#1605: introduce special RRset for in memory data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120306
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 > I'd like to hide the definition (or ideally even the existence) of
 RBNodeRRset more reliably
 I've moved it to an "internal" namespace, although I think the chance of
 this class being used out of context is negligible.

 > On a side note, assuming it's conceptually hidden and only visible to
 others as a const abstract object as the result of find() (and its
 variants) method, not supporting methods that modify the object should be
 justifiable. Maybe we want to note that fact.
 The modification methods have to be there.

 I might be misunderstanding the code, but I thought that when loading a
 zone, the RRsets and the associated RRSIGs are loaded separately.  As an
 RRset keeps a pointer to the associated RRSIG, if the RRset is loaded
 first, the object representing it has to be modified when the associated
 RRSIG is loaded in order to store the pointer to the RRSIG.

 The specification for this change called for the underlying RRSet to be
 stored and made accessible through a ConstRRsetPtr; therefore the "const"
 nature of the RRset has to be violated to store the RRSIG.

 > introduce a separate (conceptually private) non virtual methods, say,
 addRRsigInternal and removeRRsigInternal. The in-memory data source
 implementation and tests will use the latter.
 I suggest leaving that until the code is modified to use the short-cut
 additional section processing.  At the moment, RBNodeRRset is a drop-in
 replacement for RRset, which makes testing at this intermediate stage
 easier.

 > Whether it's virtual or not, we could avoid using const_cast in
 add/removeRRsig if we copy the given RRset on construction.
 True (although in that case we should change the specification to state
 that the underlying RRset is pointed to by an RRsetPtr object).  However,
 as you note, the RRset is relinquished by the loader as soon as it is
 passed into the in-memory data source.  Given this, and that the code will
 soon be modified to stop retaining the original RRset, I suggest that it's
 not worth the effort of changing it.

 > This comment is a bit ambiguous to me
 On re-reading it, it is ambiguous to me as well.  Altered.

 > I'd make the constructor 'explicit'.
 Done.  As an aside, I'm sure that many classes could benefit from an
 explicit constructor - should we start a discussion thread for this?

 > getUnderlyingRRset() can be non virtual.
 Done.

 > toWireRenderer: I'd suggest getting data (and length) directly from the
 renderer
 OK, but in that case we may want to open a ticket to change
 src/lib/dns/tests/rrset_unittest.cc, as that's where these tests were
 taken from.

 > With the suggested change above, toWireRenderer and toWireBuffer could
 be unified into a single template function (if you want)
 Good point. Done.

 > About the added test wire data: if you share test data, I'd rather add a
 data path and really share the data file than copying it.
 rrset_toWire2 has been modified because the original version included data
 from both A and NS records.  (The original test checks that rendering an A
 RRset followed by an NS RRset gives wire data containing both.) This test
 is really only to check that RBNodeRRset::toWire() forwards the call to
 RRset::toWire() object, so I removed the unnecessary part of the original
 code.

 As to the duplication of data, again I did think about that.  But because
 only one is file duplicated - and because sharing the data means that the
 tests are dependent on the test data in a different module - I did not do
 so. I think the introduction of a cross-module dependency of test data
 would cause more problems than it would solve.

 (If multiple files are duplicated, that is another matter.  But in that
 case I would suggest moving the duplicated test data into a common third
 directory.)

 > checkSignature: I'd consider using rrsetCheck
 checkSignatures replaced by a call to rrsetCheck.

 > addRRsigConstRdataPointer: what's the purpose of intermediate 'data'
 variable?
 Removed.

 > Like checkSignature above, I'd suggest considering rrsetsCheck() to
 compare two sets of RRsets.
 Done.

 Suggest the following for the !ChangeLog entry:
 {{{
 xxx.    [func]        stephen
         Add special RRset class to in-memory data source in preparation
 for
         changes concerning performance enhancements.
         (Trac #1695, git xxx)
 }}}

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