BIND 10 #408: General control logic of NSAS

[BIND 10 Development]

#408: General control logic of NSAS
---------------------------+------------------------------------------------
      Reporter:  vorner    |        Owner:  stephen  
          Type:  task      |       Status:  reviewing
      Priority:  major     |    Milestone:           
     Component:  recurser  |   Resolution:           
      Keywords:            |    Sensitive:  0        
Estimatedhours:  0.0       |        Hours:  0        
      Billable:  1         |   Totalhours:  0        
      Internal:  0         |  
---------------------------+------------------------------------------------
Changes (by vorner):

  * owner:  vorner => stephen


Comment:

 Replying to [comment:6 stephen]:
 > Is the five minutes mentioned in this method a reference to caching
 server failures? If so, the five minutes is defined in
 [http://tools.ietf.org/html/rfc2308 RFC 2308] (section 7).

 I didn't find it there. I think this is a slightly different problem ‒ the
 server itself might not be dead. I'm just unable to obtain its address
 (maybe because the nameserver that knows it is down, or because no
 nameserver I know address of knows them or because my network cable is
 suddenly eaten by a mouse...).

 > ''NameserverEntry::getAddresses()''[[BR]]
 > (Header comments) The TTL of the nameserver entry expiring while an
 address fetch is in progress should be a rare occurrence; but whatever
 happens to the nameserver entry, it should remain in place at least until
 the fetch returns.  Besides, the expiration of the nameserver entry is
 only of interest to the encompassing zone entry, not to the nameserver
 entry itself.

 I'm not sure I really understand this comment. What is the problem with
 expiring while there's a fetch in progress?

 > (Code) When checking for {{{... expiration_ <= now && expiration_ }}},
 it is marginally more efficient to reverse the order of the comparisons;
 if expiration_ is zero only one test will be peformed instead of both.

 Maybe, I'm not sure, it depends on what happens more often. But this one
 is both below any interesting speed improvement and a thing that today's
 compilers are able to understand better than humans anyway.

 Anyway, I switched it, as it seems more readable the other way.

 > (Code) In the {{{switch (getState())}}} check, under what circumstances
 would the state be IN_PROGRESS but we do not expect the address? (Am I
 right in guessing that's if there is no I/O in progress for our address
 family but there is for another?)

 I added a comment into the code. In short, yes, the entry might already
 received (or failed to) the IP address of this family while still waiting
 for the other one. In that case, we just provide them.

 > '''src/lib/nsas/nameserver_address_store.cc'''[[BR]]
 > '''src/lib/nsas/nameserver_address_store.h'''[[BR]]
 > Comment: might be a good idea to encapsulate both the hash table and LRU
 list for each of the classes (!ZoneEntry and !NameserverEntry) in one
 class. (If nothing else, it should simplify the constructor definition.)
 This could also hide the sizing of the LRU list with respect to the hash
 table size.

 I put that into a TODO list, this should go together with making multiple
 LRU lists for one hash table and with dropping LRU list of the
 NameserverEntry.

 > ''Test ChangedNS''[[BR]]
 > The header comment suggests that this does three tests.  Perhaps it
 should be split?

 They test how the entry reacts to series of events, so, no, they need to
 go one after another (and there's a part that needs to happen before this
 test, so it is tested together with it as well).

 > It's sometimes confusing to know exactly what is being tested.  Some
 better comments would help. (E.g. "Check it answers callbacks when we give
 it addresses" - I thought zone entries didn't access addresses directly.)

 They pull them out of the NameserverEntries. The resolver provides them to
 the NameserverEntry, but from the external/API point of view, it
 encapsulates handling them.

 > '''Documentation'''[[BR]]
 > It would be worth re-visiting the [wiki:NameserverAddressStoreDesign
 NSAS Design] and check that the description of the data structures and
 processing match what is now in place.

 Yes, I want to do it some time around merging. But IMO keeping
 documentation on wiki seems bad, because while the code has multiple
 versions (with possibly multiple different ways how it does something),
 the wiki shows only the latest one.

 Anything not mentioned above should already be done now.


 And I synced with #356 and did some modifications to the code merged from
 there. The biggest one is moving the address selection logic from
 NameserverEntry to ZoneEntry since we want to choose between nameservers
 as well as addresses of single one (RRT of two addresses of the same
 nameserver should be close to each other, while different nameservers
 might differ a lot).

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