BIND 10 #495: Hook up NSAS to iterator/cache

[BIND 10 Development]

#495: Hook up NSAS to iterator/cache
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  stephen
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110316
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  3.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => stephen


Comment:

 Replying to [comment:4 stephen]:
 > > it's alive! ALIVE!!!
 > Congratulations Dr Frankenstein.  Does this mean that instead of bug
 reports we'll get users carrying flaming torches hammering on the doors of
 950 Charter Street?

 as long as the angry mob goes there, i'm ok with it ;)

 >
 > '''src/bin/resolver/main.cc'''[[BR]]
 > Why is the try/catch commented out?

 arg, i was so happy i finally got it working i forgot to remove some
 changes i needed for debugging it... (on a side note, catch-alls like
 these, while nice-to-look-at from a user point of view, are really
 annoying if you want to know what is wrong, but i think i've ranted about
 that a couple of times before. This catch block is usually the first i
 remove when i need to fix something...)

 >
 > '''src/lib/asiolink/io_address.h'''[[BR]]
 > Do we need "#include <iostream>" here?  There is nothing in the header
 file that requires it.
 >

 nope, see previous point, forgot to remove it, sorry

 > '''src/lib/nsas/nameserver_address_store.{cc,h}'''[[BR]]
 > In cancel(), the "callback" argument can be passed by reference.
 >

 ack

 > '''src/lib/nsas/zone_entry.{cc,h}'''[[BR]]
 > removeCallback(): are we sure at this point that this is the only
 instance of this particular callback object in the list?  (addCallback
 just pushes the callback on the end of the callbacks_[family] vector
 without checking whether it already exists in the list.)
 >

 oh ok, i'll let it loop through the whole vector

 > The callback argument can be passed by reference.
 >
 > '''src/lib/resolve/recursive_query.{cc,h}'''[[BR]]
 > Should rename the "#ifdef" variable in recursive_query.h to
 !__RECURSIVE_QUERY_H
 >

 done

 > send(): should add a TODO to change the call to rand() to a call to a
 Boost PRNG (or a yet-to-be provided utilities "rand()" equivalent using
 Boost).
 >

 added

 > handleRecursiveAnswer(): suggest a TODO be added to the effect that
 cur_zone_ should be a "Name" object instead of a string.  (I think you
 made a comment like this in another ticket and I agree - we should try to
 avoid conversions to strings if not required.)
 >

 added (to the member declaration as well)

 > !RunningQuery constructor:  it should be possible to initialize
 nsas_callback_ with a plain "new ResolverNSASCallback(this)" and not use
 an intermediate shared_ptr.
 >

 ack, done

 > Comment.  The outstanding events pointer is fine, but is there a way we
 could handle the reference-counting semi-automatically, e.g. passing a
 shared_ptr to the !RunningQuery as an argument to the callbacks?  Then the
 object should be automatically deleted when all callbacks have fired
 without the need to maintain our own count of outstanding events.
 >

 and then put the 'final handling' (i.e. call our own callback if not done
 yet, etc), in the destructor of runningquery? Interesting, I'll need to
 think about that.

 > operator(): when calculating the elapsed time, do you need a test before
 doing the calculation?  (Also, the test is wrong - the AND should be an
 OR.)
 >

 I think so, as we don't want the difference to be negative or 0, which are
 results that are possible with gettimeofday. I think there are some clock-
 tick-dependent time functions that wouldn't have this property, but I
 don't think any of those are as portable.
 AND should indeed be OR, good catch.

 > '''recursive_query_unittest.cc'''[[BR]]
 > The test_data variable encodes the count in the first two bytes of the
 sent data.  As test_data is a byte array, the data sent should be in the
 order given.  However, the first two bytes of a TCP message are
 interpreted as a 16-bit value and converted from network byte order to
 host byte order.  Is there a chance that on some machines, the byte order
 will be flipped?  (However, in the rest of the test the first two bytes
 are not interpreted as a value, so perhaps it doesn't matter.)

 perhaps not here, but i was wondering this about our readUint16() method
 in buffer actually

 >
 > We should add some tests that check whether the cache was updated on
 successful queries (and that invalid replies were not added).

 I've added this as a TODO as well. But I'm only being half-lazy; We could
 add said check to the current disabled tests that do actual lookups, but
 even those don't work at this point (within the context of these tests,
 those lookups result in SERVFAIL), and I think we really do need that
 framework to even be able to test this right.

 Updates in commit 454739d105be01d7b9711038ca68271d0f4fd02c

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