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

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

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

 These have been done.


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

 Added.

 `master` was also merged into this branch and some tests that were deleted
 in #2292 were added back.

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