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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:13 stephen]:

 > > 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, [...]

 What I meant is that we could extend this note:

 {{{#!c++
 /// - Calls to methods that change attributes of the underlying RRset
 (such as
 ///   TTL or Name) cause an exception to be thrown.  The in-memory data
 source
 ///   does not allow modification of these attributes.
 }}}

 like this:
 {{{#!c++
 ///   does not allow modification of these attributes.  In theory, it is a
 bad
 ///   practice in that it doesn't preserve the assumed behavior of the
 base
 ///   class.  In practice, however, it should be acceptable because this
 ///   class is effectively hidden from applications and will only be given
 ///   to them as a const pointer to the base class via find() varints.
 ///   So the application cannot call non const methods anway unless it
 ///   intentionally breaks the constness.
 }}}

 As previously mentioned, I'd also add an explicit note in comments
 that this class is basically hidden:

 {{{#!c++
 /// does not have to be altered if encapsulation, rather than
 inheritcance, is
 /// used.
 ///
 /// \note This class is exposed in this separate header file so that
 /// test code can refer to its definition, and only for that purpose.
 /// Otherwise this is essentially a private class of the in-memory
 /// data source implementation, and an application shouldn't directly
 /// refer to this class.
 }}}

 > > 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?

 I thought we basically agreed about this practice, and do it in many
 cases already (I know there are exceptions, but my understanding is
 it's not intentional, just overlooked).  I even thought it's in the
 coding guideline, but I just checked and noticed it's not.  So, yes,
 it may be good to have a discussion (or just "confirmation" if my
 understanding is correct) and reflect the result in the guideline.

 > 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)
 > }}}

 I don't necessarily see the need for a changelog entry for it because
 the change is inherently invisible to users.  But I don't oppose to
 adding it, and if adding it the suggested text is okay for me (except
 that the ticket number should be #1605, not 1695:-).

 About the revised code:

 - for the iterator test I'd suggest the attached patch.  Hardcoding
   '3' is fragile in case we eventually add more records in the test.
   Also rrsetsCheck() checks if the number of RRsets is the same
   internally, so you don't have to do it by hand.

 - in addRRSIG tests of rbnode_rrset_unittest, why do you use
   getUnderlyingRRset()?
 {{{#!c++
     rrsetCheck(rrset_siga, rrset_a.getUnderlyingRRset()->getRRsig());
 }}}
   Is there a reason it cannot be rrset_a.getRRsig()?  (At least the
   latter seems to be a more straightforward conversion from the
   original code).

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