[bind10-dev] Resolver skeleton, location of logic, and plans/discussionstarter for that

[Scott Mann]

On 01/21/2011 04:06 AM, Stephen Morris wrote:
> On 20 Jan 2011, at 22:55, Scott Mann wrote:
>
>> OK. So, I've also begun working in and around this code and I certainly agree that there's a lot of stuff scattered about. Clearly, debugging would be a big issue.
>>
>> It seems that we ought to take a step back and lay out a design<->implementation relationship that says what will be where. For instance, I thought that asiolink was a utility library which would provide some sort of communication object - a udp/tcp handle or whatever - something generic and abstract. I also expected the actual query/answer objects would be elsewhere (perhaps in resolver). In short, I agree that RunningQuery should not be in asiolink. I'm less clear on whether it is a somewhat redundant object or that its function should be handled in some other object, although I highly favor simplicity.
> +1 for this.  I too thought that asiolink was supposed to be the basic I/O library which is why I was confused to find part of the resolver logic there.
>
>> Anyway, I do think a discussion is in order.
>>
>> However, I'm not sure that this it is necessary to do a pre-April discussion of this topic. Which then implies that I think that the code should be reviewed/merged and refactored later.
> Again +1
>
>> Sometimes having a poorly implemented prototype that works is a good thing.
> ...provided that it is thrown away and re-implemented in production code.  A usual problem is that prototype code ends up in the finished product because you can't afford the time/cost to rewrite it.
+1
>
> To comment a bit on Jelte's original post....
>
>> I had a brief discussion with Stephen yesterday, and we seem to disagree on
>> where this code should actually be (though I think we both agree on the 'not
>> where it is now' part, as well as on that the current general layout is
>> somewhat... misfortunate)
> I would not disagree with that statement :-)
>
>
>> Let me first describe how I read what it does, and why the resolver logic is in
>> the location it is now. That might provide a good starting point for either a
>> discussion or a refactor (or probably both).
>>
>> The resolver binary (main.cc) sets up a Resolver object (from resolver.cc),
>> which in turn creates DNSServers (tcpdns.cc and udpdns.cc) and a DNSService
>> (asiolink.cc), as well as a RecursiveQuery object (asiolink.cc).
> The start of any project is the time at which project terminology is set and the time when terminology confusion arises.  And here I am not just thinking about ourselves, but about future maintenance programmers, not to mention any customers that come to modify the code after we release it.  So if we are going to refactor the code (and documentation), we should take the time to choose terms that are unambiguous.
>
> The names "DNSServer" and "DNSService" above are a case in point - they are very similar and mean similar things.  The DNSServer class is the base of the TCPServer and UDPserver classes - perhaps renaming it to "NetworkServer" would make it clearer?
>
> I would also add that the term "query" is also ambiguous in the resolver.  Is a query the packet that the server has received from the client or the packet that it has sent to an authoritative server?  (Yes it's both, but we continually need to be clear about what we are referring to.)  Perhaps we should use the term "query" to mean the query received from the client and "fetch" for the queries made on the client's behalf?
These terms (query and fetch) seem to be used that way elsewhere, so it 
seems to be a good choice. Most importantly, we need well defined, 
unambiguous terms. So, +3 :)
>> :
>> There are a few problems, one of which is that is seems somewhat... complicated.
>>
>> An obvious conclusion is that logic and control is kind of flying around in some
>> very different places right now. Part of the handling is done in resolver.cc,
>> part in tcp/udpdns.cc and part in asiolink.cc.
> It does seem excessively complicated and it is for that reason I think we could simplify it.  I have the sneaking suspicion that stackless coroutines, introduced to simplify the code, have in fact ended up making it more complicated.
One other area of simplification should come from the integration of 
NSAS. There are a lot of data carried around in the 
RecursiveQuery/RunningQuery objects which ought to just be retrieved 
from NSAS or some other configuration source. I'm afraid I just made 
this a bit worse by adding the root ip addresses.
>> A second one is that RunningQuery does the actual recursive handling, which
>> seems to me the most logical place (right now, since that's the part getting and
>> creating events), but not everyone seems to agree :)
> This was the basis of the discussion between Jelte and myself and probably comes down to terminology.
>
>  From the names I thought that RecursiveQuery was created when a recursive query was received from a client and handled the logic of recursion, and RunningQuery was just handling the mechanics of an upstream query (sending a packet to the authoritative server, receiving a response, and handling retries on timeout).  So my understanding was that when a RunningQuery had completed, code in RecursiveQuery would get called, which might create additional RunningQuery objects (or re-use the existing one) to handle further upstream queries.
>
>> I do think we should get it out of asiolink though. And rename RecursiveQuery to
>> RecursiveQueryStarter (or Factory or something), or perhaps even remove it
>> completely. It really doesn't do much besides storing data and starting
>> RunningQueries. (another option, if possible would be to make RecursiveQuery do
>> the actual logic, but then RunningQuery would be reduced to send+timeout,
>> without even retries, in which case i'd probably argue for removing that one :))
>>
>> We should also move all the handling to a common place, whether or not that is
>> in one or more files, and certainly out of asiolink. The 'hidden' tcpdns.h and
>> udpdns.h seem strange to me as well. I want the full resolver process to be
>> callable as a library call so some things that are now in resolver.cc may need
>> to move to that common place as well (and then resolver.cc should call that).
>
> The first thing should be to step back, look at the components, and define their responsibilities.  E.g. if asiolink is the interface to ASIO - handling the sending and receiving of packets asynchronously - then that is all it should do.  Resolver objects or recursive logic has no place in it.
+1
This needs to be a high priority going in to year 3.
> Stephen
>
-Scott