BIND 10 #192: Data source hotspot cache

[BIND 10 Development]

#192: Data source hotspot cache
-------------------------+--------------------------------------------------
 Reporter:  each         |        Owner:  each                                          
     Type:  enhancement  |       Status:  reviewing                                     
 Priority:  major        |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  b10-auth     |   Resolution:                                                
 Keywords:               |    Sensitive:  0                                             
-------------------------+--------------------------------------------------

Comment(by each):

 Replying to [comment:23 jinmei]:
 > I'm (personally) not okay with merging this responsibility into the
 class responsible for name matching over data sources.  This is what I
 called "monolithic" (or leaky abstraction).  Since you seemed to disagree
 with this view, I'm giving some more detailed explanation.
 >
 > First, !NameMatch now needs to be aware of RRType and do something
 tricky in the case of the type DS and the name root on construction (btw,
 this is a new change in this branch while you seemed to say otherwise: 'It
 already had several quirks, such as the special-case code for
 rrtype==DS').

 Hmm, my mistake... I really thought that had been part of an earlier
 refactoring, sorry.

 But still...

 > Since !NameMatch is a public class that may be used in other context
 than processing an incoming query, the added mandatory parameter and the
 additional embeeded logic make unnecessarily limit the applicability of
 the class.

 What other context?  It currently returns the best enclosing zone match
 and the best data source; I really can't think of a purpose for this other
 than "figure out which zone and data source is authoritative for this
 name".  (Remember, the whole original purpose was to avoid the need for a
 Name() constructor.)

 That said, I'm fine with the separation you suggested (for the reason
 below).  But I don't see why !DataSrcMatch needs to be kept general-
 purpose, because I can't think of anything *other* than query or update
 processing that would ever need to find a zone and data source.

 > Second, !NameMatch now has both the search logic itself, even if the
 logic is only used in the top level query processing, that is, it's not
 necessary in each data source.  For example, the name() method is only
 meaningful at the top level, but is open to public, resulting in a
 slippery interface.  Consider a case where findClosestEnclosure() of a
 specific data source is called with !NameMatch (or !ZoneInfo) and it
 accidentally calls name().

 This is what sold me on the idea.  I had noticed that concern with the
 !ZoneInfo interface earlier, but it hadn't occurred to me that separating
 the functions would solve the problem.

 You said something about this being "irrelevant refactoring", though.  Are
 you saying we can do this in a different ticket?  I'm fine either way.

 > And I'd keep the ./DS consideration outside the !DataSrcMatch class and
 letting doQuery() do the job.

 Here's where I don't see the advantage anymore.  We're looking for the
 authoritative zone for a given query; the answer to that question depends
 on rrtype, so let's pass in rrtype and get the right answer.  doUpdate()
 would want the same answer as doQuery().

 > I suggest by default we should disable cache, and by default impose some
 finite limit on the # of cache entries.

 Imposing a default limit, good idea.  I'll also add a function to
 !HotCache so it can be enabled or disabled at runtime, and later on we can
 hook that into the config system.  I'd really rather not have it be
 disabled by default, though (I'd like people to *notice* the performance
 improvement :) ).

 I'm not sure what the default limit should be.  Currently on my system the
 non-caching version can serve about a thousand queries per second;
 figuring it'll be about the same if every query that comes in is a cache
 miss, we should be able to cache 30,000 names before they start expiring.
 Does that seem like a reasonable size limit?

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