BIND 10 #192: Data source hotspot cache

[BIND 10 Development]

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

Comment(by each):

 >  - I guess there's a bug in the current implementation (not specific to
 this branch.  it should be the same for the current trunk version).  see
 DISABLED_initialUpdateWithNoMatch.

 Not sure I'd exactly call it a bug, because it could never have occurred
 in normal processing, but you're right that it's an API flaw.  Thanks for
 noticing it.

 >  - I've added many more tests and more detailed doxygen description.
 I'd expect any new document to be as detailed as this one.  And, it helps
 us, too; I noticed the bug through this process.

 Thanks for that too.

 >  - and this is a substantial note: in the end, I'm feeling even
 !DataSrcMatchTest dropping RR type is leaky.  this interface cannot well
 explain test cases like !DataSrcMatchTest.updateWithWrongClass.  we could
 provide artificial restriction such as an exception to be thrown or simply
 ignoring RR class in update(), but it looks like counter intuitive and
 error prone.  To me, this suggests we should revisit the abstraction, and
 my updated suggestion is to simply go back to the original design of
 !NameMatch.

 IMHO the original design of !NameMatch isn't much better in this respect.
 I think you'll find that the initialUpdateWithNoMatch test would have
 failed before too.

 The design from day one was that there's an interplay between the update()
 function of the !NameMatch object and the findClosestEnclosure() function
 of the !DataSrc object.  They essentially ping-pong back and forth, and
 we've been treating them as if they're effectively two parts of a single
 function.

 This was always slightly odd.  Allowing uninitialized Name() objects would
 have solved the specific problem more cleanly, but introduced a different
 set of risks elsewhere in the code.  Keeping update() hidden away in
 data_source.cc as a static function or a bit of PIMPL implementation would
 have helped, but unfortunately there are lots of data sources that all
 need direct access to it, so that wasn't an option.  So, it was exposed as
 public API and yet we've treated it as conceptually static.  Oops.

 IMHO the right fix is to check the input to update().  We ought to do this
 anyway, whether we're using the old !NameMatch or the new !DataSrcMatch,
 so I don't see it as a strong argument in favor of either option.

 I'm adding a fix to the branch (r2314) which checks for class mismatch in
 all cases, and for name mismatch in the initial case.  This addresses the
 initialUpdateWithNoMatch problem.  The updateWithWrongClass test had to be
 changed because the first match now fails.  We should add some tests using
 two different data sources, one class CH and the other class IN, but if
 you'll allow it, I'd like defer that until after this branch has been
 merged so I can take advantage of your refactoring of the test data
 source.

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