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                          
Estimatedhours:                 |        Hours:                             
      Billable:  0              |   Totalhours:                             
      Internal:  0              |  
--------------------------------+-------------------------------------------
Changes (by jinmei):

  * estimatedhours:  => 0.0
  * internal:  => 0
  * billable:  => 0


Comment:

 Replying to [comment:17 each]:
 > > 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.)
 >
 > Please see r2418.

 First impression (not necessarily insisting it, but as requested):

 Clarifying the role of what was called RRsetList is good.  based on
 the clarification, I now have a slightly (or perhaps substantially)
 different opinion than that I originally stated.

 According to the clarification, the requested feature is quite limited,
 while "RRsetList" is trying to be quite generic.  I suspect the scope
 mismatch between the need and the solution is the essential cause of
 the various problems I pointed out in this ticket.

 Of course, sometimes we need to provide generic interface, and need to
 address difficulties that are often inevitable with a generic class
 (such as usage clarity or performance consideration in general case).
 But, in our case, we don't need this generality; no other part than
 the data source code uses RRsetList.  And let's not say "but someone
 may need this feature for a different purpose in the future".  We
 should know in many cases there won't be such "someone".  It should
 also be noted that if one wants to have a "set of RRsets" for some
 specific needs, one can generally use std::vector<RRset>,
 std::map<RRset>, etc.  Overall, the current implementation of
 RRsetList and attempt of fixing it as a general class doesn't seem to
 be worthwhile to me.

 With this view, using std::map internally is probably not a good idea
 (I know it was me who first mentioned it, but at that time I wasn't so
 sure if this class must be generic), because it's generally expensive
 with smaller numbers of elements.  In particular, inserting an element
 to std::map inevitably involves memory allocation (we cannot
 preallocate/reuse the room for it), it's probably not suitable in a
 performance sensitive path like the query look code path.

 I'm not so sure what it should actually look like, but my first
 impression is that what we need is a different types of RRset "tuples"
 based on your clarification:

  - Address tuples: (A, AAAA)
  - Referral tuples: (NS, DS, DNAME)
  - etc.

 where some elements of tuples can be empty.

 I'd rather consider how we reasonably model this concept, than trying
 to fix the generic "RRsetList".  I'd also move the resulting class (if
 it's a class) to the data source module.

 p.s. in any case, inheriting from std::map wouldn't be an option.
 (In my understanding) it's a generally discouraged practice.

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