BIND 10 #449: Implementation of the resolver cache

[BIND 10 Development]

#449: Implementation of the resolver cache
-------------------------------------+-------------------------------------
                 Reporter:           |                Owner:  zhanglikun
  zhanglikun                         |               Status:  reviewing
                     Type:  task     |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110208
                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 => zhanglikun


Comment:

 Reviewed commit a8746fb17e10db78330cf69b2850515020f5cd56

 '''General'''[[BR]]
 The following apply to all the files examined:

  * Return values should be enclosed in brackets according to the style
 guide.
  * A number of methods don't have the doxygen tags to describe all the
 parameters.  These should be added.
  * Comments appear to be restricted to 50 characters wide in a number of
 files.  Suggest that 80 characters would be more usual.  (This also
 appears to be the cause of some method declarations being split across
 multiple lines.)

 Comment: there is an inconsistency in how classes make themselves non-
 copyable. Some inherit from boost::noncopyable, but others take the
 approach of making the copy constructor and assignment operator private.
 Both work, but a consistent approach would be good.


 '''cache_entry_key.cc'''[[BR]]
 The function definitions (not including the return type, which according
 to the style guide should be on a line by itself) could each fit onto a
 single 80-character line - there does not appear to be the need to split
 them across multiple lines.

 The code is perhaps simple enough to inline in the header file.  It could
 be even simpler using boost::lexical_cast, i.e.:
 {{{
 const std::string
 getCachedEntryName(const std::string& namestr, const uint16_t type) {
     return (namestr + boost::lexical_cast<std::string>(type));
 }
 }}}
 This would eliminate the ".cc" file as well as remove the overhead of a
 call/return.

 '''rrset_copy.h'''[[BR]]
 '''rrset_copy.cc'''[[BR]]
 Both OK.

 Comment: strictly speaking, this class does not copy a RRset, it copies
 the Rdata (and RRSIGs) pointed to by that RRset.  However, used in
 conjunction with the initialization of "rrset_" in the RRsetEntry
 constructor, the effect is a copy.  As you note, this operation logically
 belongs in the isc::dns::RRset class.  Since this is a definite need to
 copy an RRset, I suggest that you raise this in bind10-dev, and get a
 consensus to add a copy constructor and assignment operator to that class.
 (This does not need to be done as part of this task - if consensus is
 reached, a task for it can be added to the task backlog.)

 '''message_entry.h'''[[BR]]
 Comment: the fact that the RRsetRef object is used here makes me wonder
 whether this should be part of the basic RRset class hierarchy.  The
 comments in src/lib/dns/rrset.h suggests that the design is not final and
 should be revisited.  Thoughts?

 genMessage(): the parameter description in the header suggests that
 "response" is the first argument and time_now the second.

 Presumably the reason that a number of methods are "protected" instead of
 "private" is to allow for testing?  If so, a one-line comment in the
 header file explaining that would be helpful.

 '''message_entry.cc'''[[BR]]
 getRRsetTrustLevel(): should the second argument (rrset) be passed by
 reference instead of by value to avoid a copy operation?

 getRRsetEntries(): perhaps mark a possible optimisation for the future: as
 we know that we will put at most entry_count entries in the vector,
 vector.reserve(entry_count) could be called to allocate enough space prior
 to entering the loop.

 addRRset(): should the rrset_entry_vec and section arguments be passed by
 reference instead of by value?

 addRRset(): perhaps add an "assert" to catch/log a possible programming
 error if SECTION_QUESTION is passed by mistake?

 genMessage(): the check for expiration is with "t > expire_time"; suggest
 that the ">" should be ">=".  (getRRsetEntries() checks that something has
 not expired with "t < expire_time", which leaves open the question of what
 happens if "t == expire_time"?)

 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?

 getTrustLevel(): in the processing of Message::SECTION_ANSWER, the code
 assumes that the first RRset in the section is the one that corresponds to
 the QNAME.  Some servers can return CNAME/DNAME chains, but is there
 anywhere in the protocol that stipulates the ordering of RRsets in the
 section?  It would be safer to iterate through the answer section until
 the RRset that matches the QNAME is found.

 '''rrset_entry.cc'''[[BR]]
 updateTTL(): coding standards require braces even for single line blocks.

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

 '''message_cache.h'''[[BR]]
 #include <map> is not needed, as the class does not reference a std::map
 object.  (Was this left over from an earlier iteration of the code?)

 '''message_cache.cc'''[[BR]]
 Comment: regarding the suggestion in update() for a better way to replace
 the entry.  In the short term (and for the hash table only) call add() and
 supply a third argument of "true".  A longer-term solution has been
 proposed in ticket #535.

 '''resolver_cache.h'''[[BR]]
 In the header comments there is a spurious bracket after the name of the
 exception being thrown.

 '''resolver_cache.cc'''[[BR]]
 Suggest the three-argument lookup() method be split into "lookup()" which
 checks the class and calls the second part in another method (e.g.
 "lookupInternal()") to do the actual lookup.  This would allow
 "lookupClosest()" to bypass the "classIsSupported()" call in "lookup()"
 (which does a binary search on a vector) by calling "lookupInternal()".

 Every access to one of the sub-caches (RRset, Message and !LocalZone)
 requires a lookup in a std::map, something that may be done multiple times
 in a method.  I would suggest that support for classes be done at a higher
 level - call this class something like !ResolverClassCache and call the
 containing class !ResolverCache.  When a method is called on
 !ResolverCache, it chooses a !ResolverClassCache based on the qclass then
 calls a corresponding method on that.  It simplifies the current class by
 getting rid of the indexing on the sub-caches and reduces map lookups to a
 single lookup per method call.

 '''local_zone_data.cc'''[[BR]]
 update(): the call to map::insert() followed by a call to map::erase() and
 map::insert() again can be replaced by the simpler:
 {{{
 rrsets_map_[key] = rrset_ptr;
 }}}
 (This replaces the value if "key" exists, and adds a new element if it
 doesn't.)

 '''cache_test_util.h'''[[BR]]
 Should really have header comments for these methods.

 '''local_zone_data_unittest.cc'''[[BR]]
 An assertion that the TTL is non-zero (i.e. TTL/2 != TTL) should be added.
 This is an added check that the data in message_fromWire3 is such that the
 test truly checks that the RRset has been updated.

 '''message_cache_unittest.cc'''[[BR]]
 Constructor: A qclass of 1 is IN, but RRClass::IN().getCode(), although
 more verbose, avoids making any assumptions about class values,

 testLookup: A qtype of 1 is "A", but more descriptive is using the static
 method in RRType class (i.e. RRType::A()).

 testUpdate: a qtype of 6 is "SOA", more descriptive would be to use
 RRType::SOA().

 '''resolver_cache_unittest.cc'''[[BR]]
 See comments in message_cache_unittest.cc for specifying qclass and qtype
 variables.

 '''rrset_entry_unittest.cc'''[[BR]]
 Suggest that a test be added to ensure that when the TTL drops to zero, it
 stays there and does not wrap round to a high value.  (It shouldn't with
 the current code in rrset_entry.cc, but this is an error that could be
 made if that code were modified.)

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