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:6 stephen]:
 > 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.)

 I think i got most of this.

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

 removed the boost::noncopyable ones (when there's two reasonably simple
 ways to do it i prefer the one that has no dependencies)

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

 done

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

 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

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

 +1

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

 add to discussion on copy?

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

 we'll have to ask likun

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

 ack

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

 done

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

 ack

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

 why not, added.

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

 changed

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

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

 looping over until both qname and type match

 found what i think is two errors that were tested wrong (in the case where
 a CNAME is returned *with* the target added (which is not a cname, the
 test extpected that target to be RRSET_TRUST_ANSWER_NONAA where i think it
 should have been RRSET_TRUST_ANSWER_AA)

 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

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

 done

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

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

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

 it already called add() with true, i've removed the remove()

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

 removed

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

 Right, I've renamed ResolverCache to ResolverClassCache, and made it
 specific for one class specified at creation time. Then I created a new
 ResolverCache that does its magic (now with a basic vector, some hints as
 to how to optimise better, but for now good enough i hope).

 Some code got moved, some got removed. I am undecided as of yet whether
 the ResolverClassCache should go into its own header or whether we should
 hide it completely, and only provide the 'high-level' header for
 ResolverCache. (so for now it's in the same header file, resolver_cache.h)

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

 done

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

 Done. Also renamed section_rrset_count to sectionRRsetCount. Is that
 function worthy to add to libdns++ as well?

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

 done

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

 done

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

 done

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

 done

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

 done

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

 done

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