BIND 10 #491: Connect DNS cache

[BIND 10 Development]

#491: Connect DNS cache
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  stephen
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110222
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  4.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit c9a06623e3e6041342046d52f502ce5e478ef29c
 >
 > '''src/bin/resolver/resolver.cc'''[[BR]]
 > MessageAnswer::operator(): when constructing the answer, can we assume
 that the passed-in answer_message has the RD and CD bits clear?  If not,
 the code does not explicitly clear them if the corresponding bits in the
 query message are clear. (Perhaps there should be an assertion?)
 >

 i've made it so that it'll copy those flags no matter what state they are
 in in the current answer (as I said i think the creation of messages
 warrants a bit more cleanup but i'm kind of thinking it's out of scope for
 this task)

 > '''src/lib/asiolink/recursive_query.h'''[[BR]]
 > !RunningQuery: Agree that the cache should be initialized in the
 resolver class.  The cache should be a singleton - if !RecursiveQuery is
 the code that handles an upstream fetch, placing it here suggests that
 either each upstream query has its own cache, or that the singleton cache
 is (re)initialized on every fetch.
 >

 RecursiveQuery is badly named; it evolved into the One object that Does
 Resolving. It is however reinitialized on every config change right now,
 and we do need to have direct API access to the cache eventually (for
 commands like clear cache), so putting it in Resolver would be better.
 Question is, should we do so now rather than later, or try to see exactly
 what needs to go where first :)

 (the short of it is, I think that what is not RecursiveQuery *should* be
 Resolver, and most of the functionality that is left in Resolver now
 should be moved, making Resolver (as it is named now) just pass on
 commands and config changes to what is now RecursiveQuery.

 An alternate view of this is that Resolver itself should be moved to
 lib/resolve, a new 'calling class' should be created in bin/resolver (to
 live between the world and Resolver, and pass on commands and config), and
 RecursiveQuery should have its content moved out and be removed itself)

 > '''src/lib/asiolink/recursive_query.cc'''[[BR]]
 > RunningQuery::handleRecursiveAnswer(): In the code handling
 ResponseClassifier::REFERRAL, when a glue address is found, there should
 be a "break" to exit the loop.

 ohyeah. added.

 >
 > RunningQuery::stop(); The comment is confusing - I'm not clear as to the
 "either or" - either a "full answer" (answer to the original query?) is
 cache or answer (answer to an upstream fetch?) is cached; why not both?

 our original query or our client's original query ;) (once again it would
 be useful to have some actual definitions we could use here)

 we do both now. But since the cache stores messages based on the question,
 this does mean (in normal operation) that apart from the (future) NSAS
 side-effect, we get a lot of useless rewriting in-cache (for each 'normal'
 resolver operation, we update the cache on every delegation we follow, and
 each type we more or less throw away what we stored in the previous
 roundi, at least as far as the message cache is concerned). So what i'm
 trying to convey here that (apart from NSAS side-effect), it would make
 sense to either only store 'full answers' (i.e. indeed the answers WE send
 back to our clients in response to their queries), or 'upstream answers'
 (the answers we receive from our fetches). The latter has the problem that
 we would have to re-do all processing, and the cache would only serve to
 reduce network i/o, not reduce computation (in fact it would only increase
 it)

 (as you may have guessed i see a few additions we could need in the cache,
 and i think i'll see a few more when we add the NSAS to the mix)

 I've updated the comment to better show this choice.

 >
 > RunningQuery::operator(): Should the query be placed into the cache
 before checking with !ResponseClassifier?  This could lead to erroneous
 messages being placed there.
 >

 yeah that's probably not a very good idea :)

 Moved it to handleRecursiveAnswer, and only store it if responseclassifier
 does not consider it an erroneous packet.

 > Comment: RecursiveQuery::resolver() - agree that the cache should set
 the RCODE as well.
 >
 >
 > '''src/lib/cache/message_cache.cc'''[[BR]]
 > MessageCache::lookup(): Outputs a debug message.

 removed

 >
 > MessageCache::update(): Outputs a debug message.

 removed

 >
 >
 > '''src/lib/cache/resolver_cache.cc'''[[BR]]
 > ResolverCache::update(): redundant creation of an RRClass object.
 >

 ack, removed

 > '''src/lib/resolve/resolve.cc'''[[BR]]
 > initResponseMessage(): should either assert that the question section of
 the answer section is empty before appending the question section of the
 query to it, or update the Message class to allow sections to be copied
 between messages.
 >

 right, added an assert

 > '''src/lib/resolve/resolve.h'''[[BR]]
 > In the first header for initResponseMessage, the description of
 reponse_message says "must be type Message::RENDER".  It's more accurate
 to say that the message must be in RENDER mode.

 ack, changed

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