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      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:8 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.

 I follow the style now for the opening brace.

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

 Created bug #2338 for it.

 > '''zone_data_updater.h'''
 >
 > - `ZoneDataUpdater` class needs class description doc.  The general
 >   concept, expected usage, other design notes, why it's non copyable,
 >   etc.

 These are added.

 > - `ZoneDataUpdater`: destructor is trivial and (the explicit
 >   definition) can be omitted.

 It has a definition now.

 > - `ZoneDataUpdater` constructor needs a description.

 Added.

 > - `add()` needs a description.  explain that if it can throw.

 Added.

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

 I have added `Loader`, but this is where I want to stop. After this, the
 next closest part of `InMemoryClient` contains code to populate the
 `FileNameTree`, `ZoneTable`, etc. which don't belong in a
 `ZoneDataUpdater`. I think all code left now in `InMemoryClient` belongs
 there.

 > - general: log messages are gone.  please don't make such radical
 >   changes silently (and I actually don't like to remove them).

 It was not malicious. I just thought they were redundant next to the
 `throw`s, as doing the logging when the exception is caught will still
 have the message with the problem. I've put them back.

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

 Done.

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

 Sorting is removed now.

 > - nit: technically `namespace {...}` is not "anonymous". It's an
 >   "unnamed" namespace.
 > {{{#!cpp
 > namespace { // anonymous namespace
 > }}}

 Fixed.

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

 I spent a lot of time trying to figure out why it was not being reported
 already as I'd added tests to catch such leaks (using the
 `MemorySegmentTest` class and a throw count loop). It was not creating a
 separate client for each such run, so the test was not working properly. I
 fixed the test and fixed this problem too.

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

 This is still inside `InMemoryClient` as it's used there. I've put it into
 an `internal` namespace.

 >
 > '''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());
 >     }
 > }}}

 Comment was removed.

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

 It's been renamed to `addToZoneData`, but I'd like to keep the method as a
 wrapper in case we want to add any further bits there.

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

 The tests were adjusted because I knew they were going to fail having
 written them in #2108 and this was a good way to test that the sorting was
 happening. The sorting code is gone now as we will revisit performance
 later.

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