BIND 10 #494: Modify resolver to use abstract resolver interface

[BIND 10 Development]

#494: Modify resolver to use abstract resolver interface
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  stephen
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110208
                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 jelte):

 * owner:  jelte => stephen


Comment:

 Replying to [comment:4 stephen]:
 > '''src/bin/msgq/msgq.py.in'''[[BR]]
 > Comment: it looks as if the change should have been associated with
 another ticket.
 >

 Yep, I branched off a version of trunk that happened to be broken, and
 rather than fix it in master directly and rebase (what I should've done),
 I fixed it here too, will remove this on the merge back (it's been fixed
 in master too).

 > '''src/lib/asiolink/asiolink.cc'''[[BR]]
 > !RunningQuery: references to server and server_ elements are commented
 out; should they be removed?
 >

 ack, done

 > RecursiveQuery::sendQuery() (first instance): a block of code around
 setting the resolver callback is commented out: should it be removed?
 >

 ack, done

 > '''src/lib/asiolink/asiolink.h'''[[BR]]
 > In the Doxygen headers for sendQuery, the header for the first instance
 documents five parameters (when there are two).  In the second instance,
 the name of the second parameter should be "answer_message".
 >

 ohyeah, fixed, added a reference to the callback object for more
 information (rather than repeating the information here)

 > Comment: "sendQuery()" could be better named as the brief description is
 "Initiate resolving".  How about "resolve()"?
 >

 Good point, I am a bit worried about having multiple resolve() calls
 though. But it's definitely better than sendQuery()

 > Comment: !RecursiveQuery: although documented, this does seem a bit of a
 trap for the unwary - whether the class acts as a resolver or forwarder
 depends on whether the upstream servers are set. Will there not be
 something in the configuration that controls the functionality?
 >

 Perhaps if we have the right level of abstraction (which I think we agreed
 we don't right now) we can simply make a second class that does not do the
 added processing for full resolving, and it would ignore upstream_
 completely in that case. Right now i'm not sure whether we should add a
 redundant option. I do not think that doing so would be a lot of work, but
 I do think that it would be something to propose to the team and make a
 separate task of :)

 > '''src/lib/nsas/nameserver_entry.h'''[[BR]]
 > The forward declaration of !ResolverInterface is commented out, not
 removed.
 >

 ack, done

 > '''General'''[[BR]]
 > Comment: currently no tests in src/lib/resolve, although admittedly, it
 is difficult to see what could be tested.

 Right, I have added a test now, for ResolverCallbackServer, to make sure
 it calls resume() on the DNSServer and with the correct argument.

 Quite a simple test, though making sure we had access to see whether it
 got called was a bit tricky (throwing pointers to booleans around, oh
 well).

 Oh I have taken the liberty of making one other small change, I removed
 the dlog() call in the resolver.cc ~Resolver(), which seemed quite
 unnecessary to me.

 All changes in commit 430e0e89ec5ab05bdbbd43b5fba9c93bdcf8f6c9

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