BIND 10 #55: comments on rrsetlist implementation

[BIND 10 Development]

#55: comments on rrsetlist implementation
---------------------------+------------------------------------------------
 Reporter:  jinmei         |        Owner:  each                       
     Type:  task           |       Status:  accepted                   
 Priority:  major          |    Milestone:  06. 4th Incremental Release
Component:  DNSPacket API  |   Resolution:                             
 Keywords:                 |    Sensitive:  0                          
---------------------------+------------------------------------------------
Changes (by each):

  * status:  assigned => accepted


Comment:

 I have been putting this at low priority because the code we have serves
 our needs for the moment, but I do agree with many of Jinmei's comments
 above.  As time permits I've been working on it, and I'm close to having
 some code for review.

 We do need a data structure here, but the one we have was a quick hack (by
 a C++ newbie at that) to fit the immediate needs of the code we were
 writing; it wasn't designed, as such.

 So, going back to basics:  The purpose of this class is ''short-term
 storage'' of a ''closely related collection'' of RRsets, which are the
 result of a single !QueryTask or other basic data source operation, e.g.:

  - `<qname>/A` and/or `<qname>/AAAA` for glue query tasks
  - `<qname>/NS`, `<qname>/DS` and/or `<qname>/DNAME` for referral query
 tasks
  - `<qname>/<qtype>` or `<qname>/CNAME` for normal query tasks (including
 wildcard)
  - `<targetname>/<targettype>` (or `<targetname>/CNAME`) for CNAME chasing
  - `<previousname>/NSEC` for covering-NSEC lookups
  - `<hashname>/NSEC3` for covering-NSEC3 lookups
  - `<synthname>/CNAME` for DNAME answers

 There is never a reason for a single RRsetList to contain data from more
 than one RRClass--though there are corner cases when you might not know in
 advance what the class will be.  I believe this is also true for names:
 you don't know always know what the name will be until the lookup is
 complete (as in the NSEC and NSEC3 cases, particularly), but you should
 never have an RRsetList with more than one name.

 Essentially, my proposal is to turn this class into a wrapper for
 `std::map<RRType,RRsetPtr>` instead of `std::vector<RRsetPtr>`.  (It could
 even be a derived class from the map, but for various reasons I think a
 wrapper would be cleaner and more portable.)

 I would also like to change the name, as RRsetList was always a misnomer
 (it isn't a list), but I haven't settled on a better name yet, and I think
 it's best to table that until after the design has been settled.

 In any case, its API would differ from `std::map` in a few major ways:

  - It only accepts RRsets of one RRClass, defined in advance.  If the
 initial class is ANY, then it accepts any _other_ RRClass for the first
 insertion, but then requires all subsequent insertions to match the first
 one.
  - It only accepts RRsets of one name, which may optionally be defined in
 advance.  If not defined, then it accepts any name for the first insertion
 but requires all subsequent insertions to match the first one.
  - The iterator returns values only--that is, RRsetPtr--rather than
 key/value pairs as a `std::map` ordinarily does.  (There's no particular
 architectural reason for this, but it simplifies the code in several
 places, and iterating over the keys isn't necessary.)

 I'll submit some code for discussion shortly.  (Not "for review"; we're
 not that far along yet.  I'd like to get some buy-in on the general design
 decisions, and then write a lot of documentation, and ''then'' ask for
 review.)

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