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 jinmei):

 High level comments about data_source.cc and some other related files:

 I don't think it a good idea to overload additional things onto
 !NameMatch.  It was originally intended to be a simple supplimental class
 to encapsulate the logic of searching data sources for the longest match
 name.  Now it seems to encapsulate more complecated query logic, and even
 includes a very specific trick about DS query.  It has become way too
 monolithic.

 If we really need to encapsulate the compound logic (on which I'm not
 sure, especially in the context of hot spot cache patch), we should
 consider a cleaner refactoring.  It may be a new supplemental class which
 perhaps contains !NameMatch, or it may be refactoring the logic of
 existing classes, or some other types of refactoring which may even result
 in removing !NameMatch.  But in any case, it shouldn't be making
 !NameMatch a kitchen sink monolithic class.

 As a related note, it's not clear whether the tweak for !NameMatch is
 essential for the hot spot cache feature.  Since it's already a pretty
 large patch with substantial behavioral changes, it's much easier to begin
 with a minimal diff to the existing code with new classes for the new
 feature, and then, if necessary, to refactor the resulting code.  Right
 now, the changes to the !NameMatch interfaces reqruire so many changes to
 other part of the code, including test cases, which make the review
 difficult.  And yet it's not clear these changes are inevitable for the
 new feature.

 Another high-level comment is about doQueryTask().  It has become a
 monster function, consisting of 309 lines of code with two sets of long
 switch-case blocks.  In general, it's mostly impossible to get
 comprehensive view of such a function, much less being confident about
 whether it's implemented correctly.  Also, from a quick glance many code
 fragments in the case segments to have the same pattern.  Overall, this
 function is alredy requiring a refactoring.

 At this point I'm stopping my review.  Let's resolve the high level issues
 before moving forward.  My personal suggestion is to restart with a
 minimal change that is absolutely necessary to support the hot spot cache,
 and, if necessary, think about tweaking the query logic.

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