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
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Replying to [comment:23 jinmei]:
 > 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.

 I made it return `isc::datasrc::RRsetCollectionBase` to enforce that it
 returned a datasrc implementation. It's been changed back to the base
 class now.

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

 I read the coding guidelines before making that change, and if you have
 all of the discussion you will see I mentioned to Jelte that the empty
 destructor declaration (with or without `virtual`) was unnecessary in
 the derived classes. The coding guidelines aren't always complete or
 have something I agree with, so I just follow whatever is specified for
 consistency. From the discussion, I thought it was expected that we add
 the virtual destructor to every derived class.

 Offtopic, to avoid the situation where we don't know if a `virtual`
 method is introduced by this class or some base class (last sentence of
 the coding guidelines section), we could skip using the `virtual`
 keyword and use a single line comment instead:
 {{{
 // virtual
 }}}

 > '''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.

 Nod. I'll not merge this branch till these documentation updates are made.

 > '''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.

 I've removed them now, as there's a pointer to the base class's
 documentation already, and the developer can look at it when
 implementing these methods.

 > '''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()));
 >
 > }}}

 Updated.

 > '''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.

 I've reviewed the documentation changes. What's written about the
 database result set handle becoming invalid after a commit is fine. The
 changes are fine.

 > '''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.

 These changes also look fine.

 I have made a commit on top to make minor changes.

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