BIND 10 #493: Negative cache

[BIND 10 Development]

#493: Negative cache
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  ocean
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110308
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  5.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => ocean


Comment:

 '''src/lib/cache/message_cache.{cc,h}'''[[BR]]
 Following [http://bind10.isc.org/wiki/WeeklyMinutes20110301 recent
 discussions] about future code stubs, I suggest that the stubs for the
 !MessageCache methods dump()/load() and resize() are removed.

 !MessageCache constructor: Suggest that the first and last arguments
 (rrset_cache and negative_soa_cache) are passed by reference.

 !MessageCache constructor: Suggest that the type of the first and last
 arguments are set to  RRsetCachePtr (this is defined in rrset_cache.h).




 '''src/lib/cache/message_entry.{cc,h}'''[[BR]]
 RRsetRef constructor: the "cache" parameter should be passed by reference
 and the type can be defined as a RRsetCachePtr.

 Comment: possible future optimisation: is there a need to store the
 pointer to the cache in RRsetRef as a shared_ptr?  RRsetRef is only used
 in !MessageEntry and has no existence outside it (arguably, for this
 reason it would be better defined within the !MessageEntry class).  It
 points to either the rrset_cache or the negative_soa_cache and both these
 are are kept in existence for as long as the RRsetRef exists by the
 shared_ptrs held in the !MessageEntry class.

 !MessageEntry constructor.  Both the cache parameters can be passed by
 reference (and both types set as RRsetCachePtr).

 The headerflag_aa_ flag is set from the message when the entry is
 initialized (in initMessageEntry).  The AA bit is set in the response from
 a resolver if the resolver has just read the data from an authoritative
 server, but is clear in any response that comes from cache.  At the
 moment, if the bit is set when the entry is created it remains set as
 there does not appear to be any place where it is cleared. (Perhaps it
 should be cleared in genMessage() before the new message is returned?)



 '''src/lib/cache/message_utility.{cc,h}'''[[BR]]
 hasTheRecordInAuthoritySection(): I suggest that a comment be added noting
 that you just need to know about the presence of an SOA record but don't
 know what name it will be associated with, which is why Message::hasRRset
 is not used.

 canMessageBeCached(): the criteria for caching the message is in the .cc
 file, although the .h file is usually the place for the detailed
 documentation.  Also it would be useful to mention why an NXDOMAIN
 response lacking an SOA cannot be cached.

 Comment: the methods in !ResponseClassifier seem to logically belong with
 isNegativeResponse() and hasTheRecordInAuthoritySection(), although
 lib/cache seems the wrong place for a set of general message utility
 functions.  At the next F2F we should talk about where these should live.


 '''src/lib/cache/resolver_cache.{cc,h}'''[[BR]]
 !ResolverClassCache(!CacheSizeInfo): the argument would be better passed
 as a const !CacheSizeInfo&.

 ResolverClassCache::update(const isc::dns::ConstRRsetPtr rrset_ptr): the
 argument should be passed by reference.


 '''src/lib/cache/tests/message_cache_unittest.cc'''[[BR]]
 !DerivedMessageCache constructor: suggest use of RRsetCachePtr as argument
 type instead of boost::shared_ptr<RRsetCache>


 '''src/lib/cache/tests/message_entry_unittest.cc'''[[BR]]
 !DerivedMessageEntry constructor: suggest use of RRsetCachePtr as argument
 type instead of boost::shared_ptr<RRsetCache>


 '''src/lib/cache/tests/negative_cache_unittest.cc'''[[BR]]
 testNXDOMAIN test: after the first lookup, I suggest more checks on the
 contents of returned messages (e.g. for msg_nxdomain, one could check that
 the answer and authority sections are empty, and that the authority
 section contains a single RR that is an SOA).

 testNXDOMAIN test: two queries are done for "nonexist.example.com" and
 both checks that the TTL of the SOA is 86400.  If a two-second sleep is
 inserted before the second lookup, a check could be made that the TTL is
 less than 86400 by that amount (or, to be more robust, between about 86397
 and 86399).

 Suggest tests with different SOA records:
 [http://tools.org.ietf/html/rfc2308 RFC 2308] says that the TTL of the
 negative response SOA is the minimum of the TTL of the SOA itself and the
 SOA "minimum" field.  Need three tests, with "SOA TTL" less that, equal to
 and greater than "SOA Minimum" to check that the TTL entered into the
 cache is the the correct value.  (Should also do the same for an SOA entry
 going into the normal cache to ensure that the TTL associated with the
 record is not influenced by the setting of the "minimum" value.)

 testNXDOMAINCname: need to check that the sections in the
 msg_nxdomain_cname message returned from the cache are as expected.

 testNoerrorNodata and testReferralResponse: same comments as above apply
 to the checking of the SOA TTLs and the contents of the other sections in
 the message.

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