BIND 10 #408: General control logic of NSAS

[BIND 10 Development]

#408: General control logic of NSAS
---------------------------+------------------------------------------------
      Reporter:  vorner    |        Owner:  vorner   
          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 stephen):

  * owner:  stephen => vorner


Comment:

 I have reviewed the differences between my last review (r3540) and the
 revision I checked out (r3758).

 '''src/lib/nsas/hash_table.h'''[[BR]]
 ''HashTable::getOrAdd()''[[BR]]
 Comment: the suggestion in the "to do" comment about doing part of the
 lookup using a shareable lock seems a good idea.  But I would use an
 upgradeable lock, allowing the conversion between a read lock to an
 exclusive lock without releasing it.  That would make the code simpler:
 acquire read lock and look up entry, if not found upgrade to exclusive
 lock and create entry.

 '''src/lib/nsas/zone_entry.h'''[[BR]]
 '''src/lib/nsas/zone_entry.cc'''[[BR]]

 ''newNS()''[[BR]]
 As this function is purely local to this module, suggest that it is
 declared "static"

 ''class  ZoneEntry::!ResolverCallback''[[BR]]
 Needs a header describing the purpose of the class and full header
 documentation of its methods.

 ''ZoneEntry::ResolverCallback::success''[[BR]]
 Needs better documentation.  It's manipulating pointers to nameservers,
 but I'm not clear what is the difference between "old" and "new"
 nameservers, what manipulation is taking place, or why.

 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).

 ''ZoneEntry::randIndex()''[[BR]]
 When this branch is merged with #356, check the random number generation
 code there.  It also might be worth splitting random number generating
 functions off into a separate library module.

 ''class ZoneEntry::!ProcessGuard''[[BR]]
 Needs full header documentation.

 ''class ZoneEntry::!NameserverCallback''[[BR]]
 Needs full header documentation.

 ''ZoneEntry::dispatchFailures()''[[BR]]
 Can't the arguments be passed by reference to save a copy operation?

 ''ZoneEntry::process()''[[BR]]
 The header says that this function "processes the nameservers".  What
 processing is done?

 Can't the arguments be passed by reference to save a copy operation?

 Could we have a bit more documentation on the locks.  Under what
 circumstances should the caller pass a lock and under what circumstances
 should the zone entry's own lock be used?  Also, the header says that if
 an exception is generated the lock is not released; what if the lock is
 the internal one?

 Re whether it is unsafe to lock in the middle: should process() release a
 lock if it is passed in externally?  Would it not be better to require the
 caller to unlock if process completes successfully?

 Is there really a need for the process guard?  If there are multiple calls
 on the stack, as each trigger a callback only those callbacks queued will
 be executed.  Is this a bad thing?  (If this is being called in response
 to the completion of some external fetch operation - the documentation is
 not clear here - do we want to wait until all outstanding fetches have
 completed?  Some may take a long time and some may not result in an answer
 being returned.  In this case, it is better to call all callbacks with the
 information that arrives first.)


 '''src/lib/nsas/fetchable.h'''[[BR]]
 Setting a default value of NOT_ASKED for the constructor argument would
 have saved the need to define a default constructor.


 '''src/lib/nsas/TODO'''[[BR]]
 Comment on cache notifying NSAS of changes: the only changes the NSAS
 should know about is (a) when a set of nameservers changes (this is most
 likely if the delegation RRset in the parent is different from that in the
 zone itself) and (b) if zone information is explicitly deleted from the
 cache (in which case the zone entry should be deleted).

 Comment on removal of LRU from nameserver entries: using a weak pointer in
 the LRU list is a good idea.  The nameserver entry already contains a
 pointer into the LRU list; so a simple solution is to add something to the
 destructor that removes the entry from the list.  However, it's not clear
 how to prevent a concurrent accessor looking up that nameserver entry in
 the hash table while the destruction is in progress.  The LRU list
 approach works; setting the size to about 3-4x that of the zone entry LRU
 list should avoid nameservers being deleted while their zones are active;
 few zones have more than four nameservers and many have less.

 Comment on dispatching callbacks: either remove and dispatch one at a
 time, or catch all exceptions and not if any were raised; after all
 callbacks have been dispatched if an exception was caught, raise a new
 one.  But as you say, what happens if an exception is raised in a callback
 - will the program continue?


 '''src/lib/nsas/nameserver_entry.h'''[[BR]]
 '''src/lib/nsas/nameserver_entry.cc'''[[BR]]
 Is there any reason why the header commentsfor askIP() are delimited by
 /*...*/?  All other comments in the .h file use the Doxygen "///" form.

 ''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.

 (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.

 (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?)

 ''class NameserverEntry::!ResolverCallback''[[BR]]
 Methods in this class need full documentation.  For example, what is the
 success() processing doing? (And why is the RTT being incremented if the
 current RTT is -1 (in the call to entries.push_back().)

 ''addressSelection()''[[BR]]
 This could be declared static.


 '''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.

 '''src/lib/nsas/tests/zone_entry_unittest.cc'''[[BR]]
 ''class !ZoneEntryTest''[[BR]]
 Needs better documentation, e.g. what does checkInjected() actually check?

 ''Test ChangedNS''[[BR]]
 The test name suggests that this checks for a changed nameserver, but the
 comments indicate something different.

 The header comment suggests that this does three tests.  Perhaps it should
 be split?

 ''General''[[BR]]
 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.)

 '''src/lib/nsas/tests/nsas_test.h'''[[BR]]
 The comments in class !TestResolver are delimited by /*...*/ but the rest
 of the file uses the Doxygen "///".

 '''src/lib/nsas/tests/nameserver_entry_unittest.cc'''[[BR]]
 ''class !NameserverEntryTest''[[BR]]
 The fillSet() method requires some documentation.

 '''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.

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