BIND 10 #2207: define and implement (datasrc::memory::)ZoneUpdater class

[BIND 10 Development]

#2207: define and implement (datasrc::memory::)ZoneUpdater class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  accepted
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121023
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  7
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 From a quick look, the branch generally seems to do what was intended
 in the ticket (except for the install() internal - see below).

 But on further thought, I'm now feeling the concept of load_action may
 not be really good.  To make it work for both the local and shared
 segment memory models, the caller (in this case it's
 `ConfigurableClientList`)
 needs to detect which memory model is used at the time of
 `ZoneTableSegment::getZoneUpdater()` call and provide different
 load_action callbacks.  It's against the idea of keeping the caller
 agnostic about the underlying memory model.

 Feature wise, a better behavior would be that we pass all necessary
 configuration information to `getZoneUpdater()`, and each derived
 `ZoneUpdater` class takes care of the rest of the details.  In the
 case of `ZoneUpdaterLocal`, it would:
 - it first figures out whether the zone is to be loaded from a file or
   another data source
 - if it's from a file, extract the file name from the passed config
   and invoke the parser/loader
 - if it's from another data source, (somehow) get an iterator for the
   zone of that data source, and load the data provided by the iterator
   into memory

 But then, it may not be easy/clean to implement it without making
 `ZoneUpdaterLocal` know some details of the `ConfigurableClientList`
 internal.

 If you can come up with an idea on how to do it cleanly, please make
 it happen within this branch (I've not fully thought about it and
 haven't had a specific idea).  If not, just keep the current design at
 the moment and defer the further design cleanup to a later phase.

 Some specific initial comments on the current code follow:

 - in retrospect `ZoneUpdater` (and file name of zone_updater.xx) might
   not be a good choice because it could be confused with
   `datasrc::ZoneUpdater`.  If possible, please consider less confusing
   name.
 - The latest version of #2268 introduced functions to load a zone into
   memory (either from a file or a zone iterator).  It internally
   creates a `ZoneData` and returns it with the loaded zone data.
   This would be used by the load_action callback, so, instead of
   (creating and) passing a `ZoneData` to load_action, we'd simply get
   it from these functions.  This means the signature of load_action
   should be changed by removing `ZoneData*` from the parameter and
   changing the return type from void to `ZoneData*`.  I also think
   we should pass `MemorySegment` to load_action.  At the bottom line
   these interfaces should be consistent.  So, if you think some
   part of these don't make sense, please discuss it with mukund and
   make sure these are consistent.
 - I didn't think about what should happen in the `ZoneUpdaterLocal`
   destructor, but I think cleaning up any forgotten data makes sense.
 - In `ZoneUpdaterLocal::install()`, the original intent is to update
   the zone table within this method (because this is a memory-segment
   specific detail and we want to hide it from `ConfigurableClientList`).
   In the local version, at the moment install_action() is mostly
   meaningless, so we could simply pass NULL.

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