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-20121023
  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):

 Replying to [comment:10 muks]:

 I made a few suggested changes.  Most of them are minor cleanups.
 ed2b8eb is beyond that; see below.

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

 I have multiple comments and suggestions on this part:

 - First, the loader class doesn't have to be part of the updater; the
   relationship would be rather opposite (but it doesn't have to be so
   either, so it's even better to separate them completely; in general
   it's better if two classes has less dependency between them).  I
   thought this point is non controversial, so I made suggested changes
   directly to the branch (ed2b8eb)
 - The description of `ZoneDataLoader` now generally seems to be
   obsolete (e.g., it's not an "helper internal class for load()".
   Please update.  Other public methods should have description.
 - I still suggest moving masterLoadWrapper/generateRRsetFromIterator,
   etc, to `ZoneDataLoader`.  We'll soon have to extract the zone
   loading (building a zone data instance from file/iterator) as a more
   independent step so we can separate the building/swap/cleanup steps.
   So, the `ZoneDataLoader` class should have a file name or iterator,
   have the logic like masterLoadWrapper/generateRRsetFromIterator
   internally, and provide an interface for the caller to get access
   to the built zone data.
 - To this end, `ZoneDataLoader` doesn't even have to be a class but a
   free function (or functions with different signatures) as it's a
   blocking operation:
 {{{#!cpp
 ZoneData* loadZoneData(MemorySegment&, RRClass, Name, const string&
 zone_file);
 ZoneData* loadZoneData(MemorySegment&, RRClass, Name, ZoneIterator&);
 }}}
   Although it may be a matter of preference.
 - Then the current `InMemoryClient::load()` would become like this:
 {{{#!cpp
 result::Result
 InMemoryClient::load(const isc::dns::Name& zone_name,
                      const std::string& filename)
 {
     ZoneData* zone_data = loadZoneData(mem_sgmt_, rrclass_, zone_name,
 filename);
     loadInternal(zone_name, filename, zone_data);
 }
 }}}
 - Also, I'd move the definition and declaration of the load class or
   function to a separate header and .cc.

 I'm reluctant to spend even more time for this ticket, but I suspect
 we'll need to do things like this anyway in the background loading
 work (where we need to implement the separation of zone-data
 building), so deferring that part wouldn't help much.

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

 We could discuss the need for specific log messages (and, I tend to
 agree with the ones with exceptions).  But you removed all of them
 (not only for the ones followed by exceptions), and do it silently
 (without leaving any commit comment at 371a65 or in the ticket
 comment), as part of seemingly straightforward port like 371a65.  I'd
 normally expect no behavioral change from such a big commit.  So my
 point is that if you want to introduce actual behavior change, it
 should better to be done in a separate commit and/or explicitly noted
 in some way.

 > > '''memory_client.cc'''
 > > - `InMemoryClient` constructor: Probably not due to this branch, but
 > >   it seems to be exception unsafe:
 > >   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.

 The change to the code looks okay, but it wasn't clear to me what was
 wrong with the previous tests.  Can you explain that?  Maybe as code
 comment.

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