BIND 10 #2435: implement datasrc version of RRsetCollection

BIND 10 Development do-not-reply at isc.org
Thu Jan 10 14:00:04 UTC 2013


#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:7 jinmei]:
 > '''generic'''
 >
 > - shouldn't the copyright year of new files be 2013?

 This was updated, but now the new files have been removed in later
 commits.

 > '''rrset_collection.h'''
 >
 > - maybe a matter of taste, but I'd generally like to avoid the
 >   abbreviation of "DS" to mean "data source".  "DS" is too short and
 >   could mean anything, and, in particular, it can be easily confused
 >   with the delegation signer in the context of DNS:
 > {{{#!cpp
 >     class DsIter : public RRsetCollectionBase::Iter {
 > }}}

 This was updated, but the class was removed in later commits.

 > - the relationship between the lifetimes of the updater and the
 >   collection should be documented.

 This is now documented.

 > - should we bother to even define `DsIter`?  If both getBeginning()
 >   and getEnd() just throw, this class can never even be instantiated
 >   in practice.

 Done.

 > '''rrset_collection.cc'''
 >
 > - I wouldn't leave the TODO open.  Although it wouldn't be critical
 >   for our intended usage in practice, implementing it shouldn't be
 >   difficult.  e.g. we can pass the RR class on construction as the
 >   user of the collection should know it; alternatively, we can check
 >   rrset->getClass() for the return value of find() and if doesn't
 >   match return NULL instead.

 The class is checked now.

 > - It doesn't seem to handle other minor cases correctly: what if the
 >   finder returns DNAME?  Or CNAME?  Or delegation?  And any other
 >   possible cases?  What if the wildcard flag is set even if you
 >   specify NO_WILDCARD?

 These are implemented now, but I don't know what is meant if the wildcard
 flag is set even when `NO_WILDCARD` is specified. Can the `ZoneFinder`
 return that?

 > '''database_unittest'''
 >
 > - Why does it only test it with SQLite3?  In general, our primary
 >   intent in this test .cc is to use the mock.

 It tests with both the SQLite3 accessor and the mock accessor now. But in
 one case alone (when testing the `DataSourceError` exception, we use the
 `MockAccessor` which is arranged to throw it for some test queries).

 > - It doesn't test if no-wildcard and glue-ok are really effective.

 These are tested now

 > - It doesn't test DNAME, CNAME, etc, cases (see the comments on the
 >   implementation)

 These are tested now. In the case of `DNAME`, please check the test
 (`below.dname.example.org` marked TODO) as it does not seem to be correct
 to me (or maybe I am mistaken).

 Replying to [comment:9 jinmei]:
 > A few more things:
 >
 > - `ZoneFinder::find()` throws an exception if the queried name is "out
 >   of zone".  Propagating it for the collection class is probably not a
 >   good behavior because conceptually a collection does not have to
 >   form a "zone".  Likewise, I suspect the database version of
 >   `ZoneFinder::find()` could throw `DataSourceError` for various
 >   reasons, from lower level DB error to some kind of broken setup like
 >   multiple CNAMEs.  What should we do in such cases, though?
 >   Propagating that particular exception is probably not a good idea,
 >   because the base class interface belongs to libdns++ and shouldn't
 >   know details of datasrc.  But returning NULL wouldn't be good either
 >   (unlike the case of querying "out-of-zone" name) since this is
 >   indeed an "error", not necessarily that the requested RR doesn't
 >   exist.

 This is implemented now with a `FindError` exception defined in the base
 class.

 > - what if the requested type is RRSIG?  This case can be tricky.
 > - what if it's NSEC3?
 > - what if it's ANY?  Or other meta types such as AXFR?

 These are not handled specially right now. It's documented in the base
 class.

 > And, one other thing.  I wonder it might make more sense to have
 > `ZoneUpdater` create an appropriate `RRsetCollection`, just like it
 > exposes an interface of local `ZoneFinder`.  It allows specific
 > updater implementations to customize the behavior, and maybe make it
 > easier to support iteration.

 This is done now.

 Replying to [comment:10 jinmei]:
 > If you choose to define `RRsetCollection` to be created from updater,
 > it looks just confusing to have a separate definition for
 > `datasrc::RRsetCollection`.  What we should do (or at least what I
 > envisioned) is to think about pros and cons of both approaches, pick
 > one and only one of them and implement it.

 I've moved the `datasrc::RRsetCollection` definition inside `database.cc`.

 > Also, if the decision is to extend the updater, we should generally
 > need more doc than this:
 > {{{#!cpp
 >     /// Return an RRsetCollection for the updater.
 >     virtual isc::dns::RRsetCollectionPtr getRRsetCollection() = 0;
 > }}}
 > (unless it's already planned).  Sometimes a one-liner brief doc is the
 > best we can have, but that should be very rare except for trivial
 > getter/setter.

 Done.

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


More information about the bind10-tickets mailing list