BIND 10 #2268: some cleanups for in-memory zone load

[BIND 10 Development]

#2268: some cleanups for in-memory zone load
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''general'''

 - I made a few editorial/style fixes.  I've particularly noticed you
   often misplaced braces (admittedly it's a bit odd style, though).
   See
 http://bind10.isc.org/wiki/CodingGuidelines#OpeningCurlyBracesforFunctions
   and please make sure future code will follow that.

 - regarding tests, IMO we should have dedicated tests for
   `ZoneDataUpdater` (maybe mainly by porting memory client tests)
   because in general we should test a class as direct as possible,
   minimizing other dependency.  Indirect tests often miss some cases
   (due to interface restrictions) and are harder to maintain (due to
   the additional dependency).  But in this particular case I'm okay
   with deferring that to a separate ticket.

 '''zone_data_updater.h'''

 - `ZoneDataUpdater` class needs class description doc.  The general
   concept, expected usage, other design notes, why it's non copyable,
   etc.
 - `ZoneDataUpdater`: destructor is trivial and (the explicit
   definition) can be omitted.
 - `ZoneDataUpdater` constructor needs a description.
 - `add()` needs a description.  explain that if it can throw.

 '''zone_data_updater.cc'''

 - I'd incorporate more of the load() logic here.  In fact, the
   functionality implemented in `InMemoryClient::Loader` should better
   belong here.
 - general: log messages are gone.  please don't make such radical
   changes silently (and I actually don't like to remove them).
 - `setupNSEC3`: using NSEC3Hash looks like a good idea, but I'd avoid
   creating it every time.  I guess we can make it a class member
   variable and create/initialize it only when necessary and first
   time.
 - addRdataSet(): if we want to keep the sorting and worry about the
   overhead, I guess we can relax the condition a bit and introduce
   some optimization: just sort A/AAAA/NS/SOA (and MX?) before others,
   and simplify and fasten the comparison logic using bitmaps (we only
   need 32 bits).
 - nit: technically `namespace {...}` is not "anonymous". It's an
   "unnamed" namespace.
 {{{#!cpp
 namespace { // anonymous namespace
 }}}

 '''memory_client.cc'''
 - `InMemoryClient` constructor: Probably not due to this branch, but
   it seems to be exception unsafe:
 {{{#!cpp
 InMemoryClient::InMemoryClient(util::MemorySegment& mem_sgmt,
                                RRClass rrclass) :
     mem_sgmt_(mem_sgmt),
     rrclass_(rrclass),
     zone_count_(0),
     zone_table_(ZoneTable::create(mem_sgmt_, rrclass)),
     file_name_tree_(FileNameTree::create(mem_sgmt_, false))
 }}}
   when `FileNameTree::create()` throws.

 '''memory_client.h'''

 - The `LoadCallback` type is essentially private.  I'd clarify that by
   putting it into a separate namespace (e.g. "internal").  But, this
   should probably go to zone data updater (see above) in the first
   place.

 '''zone_finder_unittest.cc'''

 - The comment doesn't make sense any more:
 {{{#!cpp
     // simplified version of 'loading' data
     void addZoneData(const ConstRRsetPtr rrset) {
         updater_.add(rrset, rrset->getRRsig());
     }
 }}}
   Also, we might just deprecate entire addZoneData() as its content is
   a simple one liner (for that matter, `addZoneData()` doesn't sound
   like a good name anyway - it should be named something like
   `addRRsetToZone()`)
 - If we keep the per-type sorting, I think there should be more tests.
   For example, what if we add NS then NSEC?  If NSEC then NS?  Case
   with more than two RRsets, etc.  Also, such part should have been
   written test driven: write some new test case, confirm it fails, and
   incrementally add the sorting logic (b3d2730 doesn't seem to be like
   that - it more suggests that the whole main code was written and
   then failed existing tests were adjusted).

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