BIND 10 #2435: implement datasrc version of RRsetCollection

[BIND 10 Development]

#2435: implement datasrc version of RRsetCollection
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:22 muks]:

 > > '''datasrc/rrset_collection_base.h'''
 > >
 > > - This shouldn't be necessary if you include zone.h:
 > > {{{#!cpp
 > > /// \brief A forward declaration
 > > class ZoneUpdater;
 > > }}}
 >
 > It seems to be necessary (which is why it was added). Both classes
 > refer to each other.

 Ah, okay.  And this reminds of one thing I wondered about previously:
 I guess the return type of `getRRsetCollection()` should better be
 `dns::RRsetCollectionBase`, because the application should basically
 (only) use the highest level of interface (with noted restrictions).
 Also, `datasrc::RRsetCollectionBase` should basically be considered
 an internal definition that only the implementation of
 `RRsetCollection`s should refer to, so it's not really good if it's
 exposed via the public interface of updater.

 Then `ZoneUpdater` won't depend on `datasrc::RRsetCollectionBase`.

 > > - destructor doesn't have to be defined explicitly in this case.
 >
 > There was a discussion on Jabber about 2 weeks back on whether to
 > include empty destructors anyway in derived classes to show up the
 > `virtual` keyword (though it's unnecessary). I don't know what the
 > coding style is here, so based on that discussion I added it.

 I wasn't in that discussion at the exact time and missed some part of
 it, but according to the very last part of it it's more about this:
 http://bind10.isc.org/wiki/CodingGuidelines#VirtualMethods
 rather than whether to explicitly define empty derived destructors.

 > > '''zone.h'''
 > >
 > > - not necessary:
 > > {{{#!cpp
 > > /// \brief A forward declaration
 > > class RRsetCollectionBase;
 > > }}}
 >
 > Please see the comment above.

 Okay, and please see my response to it:-)

 Comments on the revised branch follow:

 '''note before merge'''

 We'll need to update pydoc for corresponding Python wrappers when the
 C++ doc is fixed.  I plan to provide a patch for that at the final
 stage of the review once all outstanding issues (at least on doc) are
 resolved.

 '''rrset_collection_base.h'''

 - `getBeginning` and `getEnd` doc: it's not clear (if you don't know
   the dev history) why both exceptions are listed.  I'd clarify it or
   simply remove `isc::dns::RRsetCollectionError` for now.

 '''database_unittest.cc'''

 - I actually meant doing some test with find (even though out of the
   scope for this case).  for example:
 {{{#!cpp
     // Just call getRRsetCollection() here. The test using .find() is
     // unnecessary for the purpose of this test case, but we have it to
 use
     // the result of getRRsetCollection() and silence some compiler
 complaining
     // about ignoring the return value of getRRsetCollection().
     EXPECT_FALSE(this->updater_->getRRsetCollection().
                  find(Name("www.example.org"), RRClass::IN(),
 RRType::MX()));

 }}}

 '''zone.h'''

 - The description for `getRRsetCollection` still doesn't seem to cover
   everything I'd envision.  For example, it doesn't explain why we
   provide overwrapping interface of getRRsetCollection() and
   getFinder().  The paragraph is also not readable because multiple
   different things are put there.  I'm attaching my suggested text to
   address these concerns in the form of diff.  Please review, apply
   and/or discuss it.

 '''dns/rrset_collection_base.h'''

 - The doc looks a bit awkward in a similar sense as discussed for
   zone.h.  The paragraph for the RRSIG explanation is particularly odd
   because it mentions NSEC3 with no further explanation of it.
   Also, I don't think we have to mention the specific behavior of
   `dns::RRsetCollection()`: just mentioning possibilities of such
   cases should be enough.  One other point: the mixture of "concrete
   type" and "meta type" is confusing.  I'm including my proposed
   changes to address these concerns in the diff I'm attaching.

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