BIND 10 #449: Implementation of the resolver cache

[BIND 10 Development]

#449: Implementation of the resolver cache
-------------------------------------+-------------------------------------
                 Reporter:           |                Owner:  jelte
  zhanglikun                         |               Status:  reviewing
                     Type:  task     |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110222
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  8.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jelte


Comment:

 Reviewed commit a02e4f3441285f10958b72e2337aa1b6e290fb56

 > removed the boost::noncopyable ones (when there's two reasonably simple
 ways to do it
 > i prefer the one that has no dependencies)
 Agreed.  The boost::noncopyable appears to be a good idea but as it does
 involve bringing in a bit of boost into the header (with all that
 entails), defining the copy constructor and assignment operator as private
 seems to be the best option.  Should we update the coding guidelines with
 this?


 '''src/lib/cache/cache_entry_key.cc'''[[BR]]
 >> The code is perhaps simple enough to inline in the header file. It
 could be even
 >> simpler using boost::lexical_cast, i.e.:
 >> :
 > true, but I'd like use to try and reduce the usage of boost in our
 headers, so for
 > now i'm leaving this as is.
 Fair enough about the headers.  However even in the .cc file I think the
 simplification would make the code clearer.  But since it works as is
 there's no urgent need to change it.


 '''src/lib/cache/message_entry.cc'''[[BR]]
 getRRsetTrustLevel(): the addition of the code to locate the correct RRset
 is a good idea.  Perhaps add an assertion after the loop that the RRset is
 found (i.e. rrset_iter != message.endSection(section))?

 >> genMessage(): to check if any of the RRsets in the message have
 expired, getRRsetEntries() is
 >> called. Although only the return status is used, a vector of
 RRsetEntryPtrs is created in the
 >> process which is discarded without being used. The
 construction/destruction of the vector is
 >> additional overhead; perhaps a method should be added purely to check
 if a message has expired?
 > hmm, not sure if that would cause extra overhead, since that vector is
 used if nothing has
 > expired. (i don't know which would be the most common path)
 You're right, ignore my comment.

 > I don't know the specific way this code is called yet, but i suspect
 that doing the comparison
 > by pointer is not the right thing here and we should have an equals() in
 RRset (which we
 > don't), or we should explicelty and loudly put in the api docs that you
 are only to use this
 > code with the same pointers
 This is true.  I think that this is loudly arguing for modification of
 RRset.  The earlier comments noted that there was a need for a copy
 constructor and assignment operator, and this is suggesting that we also
 need a comparison operator.  As these extend and not alter RRset, I have
 just gone ahead and added task #568 for it.


 '''src/lib/cache/resolver_cache.cc'''[[BR]]
 !ResolverClassCache: debug messages still present in some of the methods.

 ResolverClassCache::updateRRsetCache(): the RR type is converted to a
 string and then checked against the strings "A" or "AAAA".  I suggest
 avoiding the string conversion by comparing the RRType() returned by
 getType() directly against RRType::A() and RRType::AAAA()? (RRType
 contains operator==().)

 Comment: ResolverCache::lookupClosestRRset() - by changing the "if (!cc)"
 check to "if (cc)", and enclosing the bulk of the code in the "if" braces,
 the first "return (RRsetPtr())" can be removed - the case where "cc" is
 NULL will be handled by the final return statement. (Also - although the
 optimiser and processor branch prediction logic will probably handle this
 - the common case where the "cc" is non-null avoids taking one of the
 branches.)  A similar comment applies to the update() methods.


 '''src/lib/cache/rrset_cache.cc'''[[BR]]
 >> update(): Need to add a "TODO" that if the RRset being updated is an NS
 RRset,
 >> the NSAS should be updated as well.
 > er, but not from here i hope
 Comment: Where should it be done?  The idea here was based on the idea
 that the resolver code should only go to the NSAS to get the address of a
 nameserver for a zone, it should not need to worry about maintaining the
 data in it.  So when the resolver updates the cache, it is the cache that
 updates the NSAS.  But I'm open to suggestions.


 '''src/lib/cache/tests/cache_test_util.h'''[[BR]]
 > Also renamed section_rrset_count to sectionRRsetCount. Is that function
 worthy to add to libdns++ as well?
 +1

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