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

 Regarding cache_unittest.cc, you appear to have already addressed at least
 two of your comments in the branch, and maybe all three.  Does that need
 any further attention from me?

 Replying to [comment:14 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.

 A note on that:  I have kept the class name as "!NameMatch" for the
 moment, because that was the class that I had extended, but my intention
 was to change its name to !ZoneInfo or something similar.  The only reason
 I didn't do this already was that I thought it would be slightly easier
 for the reviewer to follow the logic if it was still obvious that I had
 based the new code on an existing class.  (I thought I had mentioned this
 in the comments, but I probably removed that by mistake during cleanup;
 sorry.)

 > If we really need to encapsulate the compound logic (on which I'm not
 sure, especially in the context of hot spot cache patch)

 I believe we do.  Otherwise every query still requires at least one
 database access to find the concrete data source before anything else can
 happen.  That one change, keeping track of the data source in which the
 best enclosing zone had been found, massively improved the query
 performance.

 > 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 I already mentioned, my intention was to change !NameMatch to a new
 name.  !NameMatch has no purpose other than this one, so I don't there's
 any reason to keep it around in its old form.  It's not a kitchen-sink
 monolithic class; it's a fairly straightforward structure for determining
 the best matching zone for a given query, and the data source in which the
 zone resides.

 > 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.

 That's a fair criticism.  The fragments do generally differ in their
 patterns, for good reasons; I eliminated code duplication when I could,
 but the behavior of a GLUE_QUERY is different from an AUTH_QUERY in
 critical ways.

 However, it can be split into smaller service functions handling the
 separate query task types.  I'm accustomed to doing this last, after
 satisfying myself that the overall structure is correct (you may recall
 that we went through a similar process with doQuery() earlier).

 If you insist on stopping the review until this has been done, okay.  But
 if you could treat each switch block branch as a separate logical unit and
 carry on, or else just pass over doQueryTask() entirely and continue
 reviewing other things, that would make things easier when I have time to
 work on this again next week.

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