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

 Replying to [comment:22 each]:

 > > I see the effect of checking the cache before trying to identify the
 best data source. But that doesn't mean you have to alter the definition
 and interfaces of an existing class (NameMatch), affecting many other
 parts of code that have nothing to do with hot spot caching. That was my
 point.
 >
 > If you prefer, you could think of it as eliminating a class that's no
 > longer needed (!NameMatch) and replacing it with one that serves our
 > needs better (!ZoneInfo).
 >
 > The !NameMatch class is *not* a pristine general-purpose class; its sole
 > function, prior to this change, was the one it's still being used for
 now:
 > to iterate through through a list of data sources looking for the one
 that
 > is authoritative for a given name.  There is no other use to which it
 can
 > be put.  It already had several quirks, such as the special-case code
 for
 > rrtype==DS.
 >
 > I haven't really changed it, I've just given it a slightly broader
 remit,
 > in order to eliminate unnecessary database searches.  I don't think
 there's
 > any simpler way to accomplish this that still makes sense.

 I've been partially convinced, but not fully:-)

 I'm okay with encapsulating zone related information (along with its data
 source) in a separate class and having the class the search logic.

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

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

 So, I'd suggest separating these responsibilities, and hiding the matching
 logic class within the data_source.cc implementation.  For example, it
 would be something like this:

 {{{
 class DataSrcMatch {
     DataSrcMatch(const RRClass& rrclass, const Name& qname);
     ~DataSrcMatch();
     const RRClass& getClass();
     const Name& getQname() const { return (qname_); }
     const Name* getClosestName() const { return (closest_name_); }
     const DataSrc* getBestDataSrc() const { return (best_source_); }
     void update();
 };

 class ZoneInfo() {
     const Name* getName();
     const DataSRc* getDataSrc();
     DataSrcMatch datasrc_match_;
 };

 ZoneInfo::getName() {
     if (datasrc_match_.getClosestName() != NULL) {
         toplevel_datasourc.findClosestEnclosure(datasrc_match_);
     }
     return (datasrc_match_.getClosestName());
 }

 ZoneInfo::getDataSrc() {
     if (datasrc_match_.getBestDataSrc() != NULL) {
         toplevel_datasourc.findClosestEnclosure(datasrc_match_);
     }
     return (datasrc_match_.getBestDataSrc());
 }
 }}}

 where I renamed !NameMatch to !DataSrcMatch, because this is actually what
 the original !NameMatch was supposed to be, and we could then avoid
 passing the class to findClosestEnclosure() separately.  But it may be
 better to defer this change as it's now truely an irrelevant refactoring.
 Likewise, I changed some "getter" method names to be aligned with our
 coding guideline, but this is probably better to be deferred.

 !ZoneInfo is intended to be a private class, declared and defined within
 data_source.cc (not in a public header file).

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

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