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:

 Hi Jinmei

 Replying to [comment:20 jinmei]:
 > I think the interface is now okay.  Remaining issues are mostly
 > editorial/documentation/cleanup.  So I suggest merging the branch at
 > this point so other tasks can move forward, and address the remaining
 > issues separately.

 The `trac2435` branch was merged to `master`. The following comments
 apply to `trac2435_2` which was freshly branched from `master`.

 > As for `RRsetCollectionPtr`, I'd personally rather remove it based on
 > YAGNI, unless we are *sure* we'll need it in a near future update.

 Removed.

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

 > - maybe a matter of wording, but to me this `RRsetCollectionBase`
 >   looks like the "default implementation" rather than an abstract
 >   class.
 > {{{#!cpp
 > /// This is an abstract class that adds datasrc related detail to
 > /// \c isc::dns::RRsetCollectionBase. Derived classes need to complete
 > /// the implementation (add iterator support, etc.) before using it.
 > }}}
 >   Although it's not clear what we'll need to do for the iterator case,
 >   it may still be possible to provide the default implementation only
 >   using public updater interfaces.
 > - do `getBeginning` and `getEnd` have to be declared as pure virtual?
 >   These are inherited from the very base class and are already pure
 >   virtual.  It may also be related to the question of "default
 >   implementation" vs "abstract class" (above).  Especially if we
 >   consider it the former, I guess we'd rather add the default
 >   implementation (which currently just throws) in this "base" class.

 It was abstract before, but I've updated it now as suggested to include
 the iterator stub implementation.

 > - documentation of this class (design decisions, restrictions, how
 >   it's expected to be used, etc) still seems to be insufficient.  in
 >   general, it should contain the detailed points we've discussed in
 >   this ticket.

 This has been added, and it also points to `ZoneUpdater` class doc to
 avoid duplication.

 > '''database.cc'''
 >
 > - this is not very accurate:
 > {{{#!cpp
 > /// \brief datasrc implementation of RRsetCollectionBase.
 > }}}
 >   it's specific to DBs

 Updated.

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

 It's removed now.

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

 Please see the comment above.

 > - I'd like to see an explanation of why we have this restriction:
 > {{{#!cpp
 >     /// Implementations of \c ZoneUpdater may not allow adding or
 >     /// deleting RRsets after \c getRRsetCollection() is called.
 > }}}
 > - Likewise.  And also, if this means the user of the base class
 >   shouldn't use the collection after commit (unless it knows a
 >   particular derived implementation that loosens the restriction),
 >   I'd note that explicitly.
 > {{{#!cpp
 >     /// Implementations of \c ZoneUpdater may disable a previously
 >     /// returned \c RRsetCollection after \c commit() is called. If an
 >     /// \c RRsetCollection is disabled, using methods such as \c find()
 >     /// and using its iterator would cause an exception to be
 >     /// thrown. See \c isc::datasrc::RRsetCollectionBase for details.
 > }}}
 > - Likewise as the above two points.
 > {{{#!cpp
 >     /// Implementations of \c ZoneUpdater may not allow adding or
 >     /// deleting RRsets after \c getRRsetCollection() is called. In this
 >     /// case, implementations throw an \c InvalidOperation exception.
 > }}}
 >   (There are two copies of this text, btw)

 These have been addressed too.

 > '''database_unittest.cc'''
 >
 > - I'd avoid C-style cast:
 > {{{#!cpp
 >     (void) this->updater_->getRRsetCollection();
 > }}}
 >   In this case you could probably use a simple (though redundant in
 >   some sense) test using find(), for example.

 Updated using a `find()` call as suggested.

 > ''' (dns)rrset_collection_base.h'''
 >
 > - s/an/a/ ?
 > {{{#!cpp
 > /// This exception is thrown when an calling implementation of
 > }}}

 It was meant to read "... when calling an implementation...". This has
 been fixed.

 > - I suspect this is not the main difficulty of RRSIG.  One fundamental
 >   issue is that it's not clear whether we want to return all RRSIGs
 >   of the given name covering any RR types (in which case how), or we
 >   need to extend the interface so we can specify the covered type.
 >   Also, I suspect the simple libdns++ version of collection could
 >   actually match RRSIG RRsets because its add interface doesn't reject
 >   the direction addition of RRSIGs.  So, what should be documented is
 >   that we have such fundamental open questions, and that a specific
 >   derived implementation may happen to return something if type RRSIG
 >   is specified, but nothing can be assumed as the base class interface
 >   (and we might say we may clarify and implement it in the future)
 > {{{#!cpp
 >     /// This method's implementations currently are not specified to
 >     /// handle \c RRTypes such as RRSIG and NSEC3. RRSIGs are attached
 >     /// to their corresponding \c RRset and it is not straightforward to
 >     /// search for them. Searching for RRSIGs will return \c false
 >     /// always. Support for RRSIGs may be added in the future.
 > }}}

 This comment has been updated.

 > - This doesn't seem to be very accurate.
 > {{{#!cpp
 >     /// Non-concrete types such as ANY and AXFR are unsupported and will
 >     /// return \c false always.
 > }}}
 >   If that's the case, we should implement derived classes that way and
 >   test the behavior (but I don't think it's the case).  Similar to the
 >   case of RRSIG, the fact is that specific implementation may return
 >   something for these (we can create a type AXFR RRset and pass it to
 >   libdns++ collection, for example).  But, unlike the case of RRSIGs,
 >   these types of RRsets are not expected to be included in any
 >   implementation of collection in the first place (by definition of
 >   "meta types"), so asking for such types is basically an invalid
 >   operation.  While the API doesn't require implementations check this
 >   condition (and reject it), it should be considered an "undefined
 >   behavior".  (and that wouldn't change in future versions).

 This comment has been updated.

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