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-20130122
         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:

 Hi Jinmei

 Replying to [comment:16 jinmei]:
 > '''The derived RRsetCollection class implementation'''
 >
 > At least for now, it's not specific to the DB-based data source, and,
 > would work for an in-memory updater if/when we implement it, too.  It
 > may even be the case for a future version with the iteration support.
 > So I'd introduce an intermediate base collection class that works as
 > the default for all data source implementations.

 This has been introduced.

 > Also, as partly discussed above, this `RRsetCollection` will have non
 > trivial restrictions, not only that it shouldn't be used after the
 > originating updater is destroyed:
 >
 > - Strange things will happen if more changes are made after the
 >   collection is constructed, especially about the iterator.  One way
 >   to prevent it is to forbid further updates once `RRsetCollection` is
 >   created.  At least for the purpose of post-load check, this
 >   restriction is acceptable.

 Addition and deletion throw now, after an RRsetCollection is requested
 from the `DatabaseUpdater`.

 > - I suspect it won't work after commit, even if the updater is still
 >   alive.

 `DatabaseUpdater::commit()` now disables the database `RRsetCollection`.

 > - use of multiple instances of collection may have some restrictions.
 >   for example, iterator can work in only one of them in the case of
 >   DB-based data sources; on the other hand, in-memory version probably
 >   doesn't have this restriction.

 I have chosen not to implement multi-instance capability for now.
 This is because we don't know yet if such a multi-instance feature will
 be useful. For now, the `ZoneUpdater::getRRsetCollection()` implements
 the stricter interface and returns an object with the
 `isc::datasrc::RRsetCollectionBase&` interface.

 > '''database_unittest.cc'''
 >
 > - about the TODO: I think in this case it's okay.  But this affects
 >   the characteristic of the datasrc ('s updater) version of
 >   `RRsetCollection`.  That is, this collection consists of RRs of the
 >   corresponding zones, including "glue" RRs below a delegation zone
 >   cut, but not external ones like those subject to DNAME substitution
 >   or complete outside of the zone's name space.  So, specifically, not
 >   all records added through the updater may necessarily be found by
 >   the collection's find().  That should be documented.
 > {{{#!cpp
 >     // TODO: "below.dname.example.org." with type A does not return the
 >     // record (see top of file). It needs to be checked if this is what
 >     // we want.
 >     rrset = this->collection->find(Name("below.dname.example.org"),
 >                                    this->qclass_, RRType::A());
 >     // Is this correct behavior?
 >     EXPECT_FALSE(rrset);
 > }}}

 Updated.

 > '''rrset_collection_base.h'''
 >
 > - The `FindError` class: I've seen mixed styles for exception classes
 >   mainly used with a particular class (like this): define exception
 >   class as the main class and define the exception outside of it.
 >   Previously I didn't have a strong opinion either way except that the
 >   style should be unified, but I'm now more inclined to the latter
 >   style.  A subclass like this knows too much internal detail of the
 >   enclosing class; it's effectively defined as a friend of the
 >   encloser and has all the power of the friend, referring to or
 >   modifying its private variables, etc.  Obviously that's too much for
 >   exception.  All we need is to clarify the context relationship
 >   between these two, and the usual tool for it is namespaces.
 >   (Unfortunately) we normally do not use a separate namespace per
 >   class basis, so we cannot take that approach, but I don't think this
 >   can be used to allow such intrusion of class integrity.  So, in
 >   conclusion, I suggest we define this exception outside of
 >   `RRsetCollectionBase` and name it something like
 >   `RRsetCollectionError`.  (We should also discuss it and eventual
 >   unification of the style, but that's beyond the scope of this task).

 Done.

 > - This doesn't seem to be sufficient.  What happens if these types is
 >   given?  Why can't we handle them?  What will happen in future
 >   versions?
 > {{{#!cpp
 >     /// This method's implementations currently are not specified to
 >     /// handle \c RRTypes such as RRSIG, NSEC3, ANY, or AXFR.
 > }}}

 The doc has be made more elaborate.

 > - This needs documentation.
 > {{{#!cpp
 > typedef boost::shared_ptr<RRsetCollectionBase> RRsetCollectionPtr;
 > }}}

 Documented, and also moved to `isc::datasrc::RRsetCollectionBase`.
 It is no longer used in our code now, but I think it may be used
 again in the memory datasrc implementation, so I've left it in
 for now.

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