BIND 10 #449: Implementation of the resolver cache

[BIND 10 Development]

#449: Implementation of the resolver cache
-------------------------------------+-------------------------------------
                 Reporter:           |                Owner:  stephen
  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 jelte):

 * owner:  jelte => stephen


Comment:

 Replying to [comment:10 stephen]:
 > 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?

 sent a mail to -dev to ask if anyone has any opinion on this

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

 ack, done.


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

 ok cool. Ignoring for now :)

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

 removed

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

 oh i completely read over that... fixed

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

 ack, changed

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

 hmm, you are probably right. I was kind of hoping to avoid a circular code
 dependency (so that the cache would not necessarily need to know about the
 NSAS and vice versa), but thinking about it, I don't know how to avoid it
 right now. Added todo.

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