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

[BIND 10 Development]

#2207: define and implement (datasrc::memory::)ZoneUpdater class
-------------------------------------+-------------------------------------
                   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:  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):

 Replying to [comment:7 vorner]:

 > > 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.
 >
 > I don't think the interface itself is problematic. How I'd see it is:
 >  * With the Local versions, we pass the load_action, it used to load
 data into
 >    the memory. Just as it is now.
 >  * In case of the shared memory, the load_action is ignored. The updater
 (now
 >    renamed to reloader) asks the memory manager for the data by the
 origin and
 >    class, then waits for confirmation and maps the shared memory to the
 address
 >    space.
 >  * In the memory manager, we just reuse the „local“ reloader on top of
 the
 >    shared segment.
 >
 > Or did I miss something why this couldn't work?

 Hmm, it sounds not as bad as I previously thought.  I cannot be sure
 until we actually try to cover other cases like the share memory
 version, but for now I'm fine with this.

 > > - 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.
 >
 > Renamed to ZoneReloader. Is it better?

 "*re*loader" still doesn't sound like a good choice to me because
 it'll also be used for the initial load (although in that sense
 `datasrc::ZoneUpdater` may be suboptimal, too).  `ZoneWriter` just
 occurred to me - how about that?  Basically not so different from
 "updater" but just to have a different name so it's less confusing.

 > > - In `ZoneUpdaterLocal::install()`, the original intent is to update
 > >   the zone table within this method (because this is a local-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.
 >
 > Thinking about that, I just removed the install_action completely.
 There's no
 > need for the local reloader to take it as a parameter on construction
 and then
 > not use it. If there's need for one later on in other reloaders, the
 factory
 > function (`getZoneReloader`) will just get another parameter.

 Okay, but there's still some important gap between what I originally
 intended and the actual implementation (probably because the ticket
 description wasn't clear enough).  See specific comments below.

 '''general'''

 - on the interface design: maybe just expose the same single
   method, like doNextStep(), instead of load()/install()/cleanup()?
   In fact, the caller doesn't have to know what these methods are
   supposed to do; the only important point for the caller is whether
   the next step must be considered a critical section (when called in
   multi-thread environment).  So, one idea would be to provide a
   single method that returns whether there's still more step(s) and in
   that case whether it must be in a critical section (and the first
   call would have to be guaranteed to be thread safe).  Then the
   application code would look like this:
 {{{#!cpp
     ZoneReloader* reloader;     // get from somewhere
     // result.first = need more step; result.second = need mutex for next
 step
     std::pair<bool, bool> result(true, false);
     while (result.first) {
         if (result.second) {
             Locker locker(mutex);
             result = reloader->doNextStep();
         } else {
             result = reloader->doNextStep();
         }
     }
 }}}
   This way, we can give the specific derived reloader more freedom of
   skipping some of the steps or adding extra steps, and neither the
   reloader nor the application has to worry about most of the call
   ordering issues.  But I'm going to complete my review below based on
   the current interfaces.
 - this is not in the guideline, but I though we have general agreement
   about adding a blank line before the `\brief` comment line:
 {{{#!cpp
     /// \throw isc::Unexpected if called second time.
     virtual void load() = 0;
     /// \brief Put the changes to effect.
 }}}
   because without explicit separation it's not immediately clear which
   method the description is for.  I actually would like to propose
   including this in the guideline.

 '''load_action.h'''

 I'm afraid it's not clear specifically how this type is used just from
 this documentation.  For example, it doesn't explain how an
 implementation of this type gets the source of the zone data
 (e.g. zone name, master file name).

 '''zone_reloader.h/cc'''

 - I'd request derived class implementation provide the strong
   exception guarantee, i.e, when any of the methods throws, the
   underlying data should be kept in the state before the method call
   (either by not making actual change until it's safe or by safely
   rolling back to the original state) as part of the API contract.
 - I'd hide `ZoneReloaderLocal` in some a "private" header (not in the
   public zone_reloader.h)

 - I suspect `segment` parameter of the `ZoneReloaderLocal` constructor
   should be of `ZoneTableSegmentLocal`, and we can safely do that
   because this class should be constructed only by a
   `ZoneTableSegmentLocal` object.

 - On a related note, `ZoneReloaderLocal` needs some way to get access
   to the `ZoneTable` that should be stored in `ZoneTableSegmentLocal`
   because otherwise it cannot update an existing table for reloading
   one of the zones in it (see also comment on install() below).

 - `ZoneReloaderLocal::load()` description: this seems to be an older
   version of description; it doesn't prepare an empty `ZoneData`
   by itself anymore.  Same for install().

 - `ZoneReloaderLocal` implementation in general: I'd throw something
   different from `Unexpected` to indicate a caller's misbehavior (such
   as passing an invalid parameter or calling a specific method at a
   bad timing).  `Unexpected` was intended to mean really unexpected
   event like an unexpected system error.

 - `ZoneReloaderLocal` general: are both loaded_ and data_ready_
   necessary?

 - `ZoneReloaderLocal::install()` implementation: the zone table is not
   expected to be created here, but is passed from the
   `ZoneTableSegmentLocal` that created `ZoneReloaderLocal` (either as
   `ZoneTableSegmentLocal` itself or as an explicit parameter).

 '''zone_table_segment_local'''

 - Not really for this branch, but if we return bare pointer via
   getZoneReloader(), we should clarify who is responsible for freeing
   it in the (base class) documentation.

 '''zone_table_segment.h'''

 - The `table` member should be set on the construction of
   `ZoneTableHeader`, and like things like `ZoneData` or `RdataSet`
   it would only be created and destroyed via separate static
   create()/destroy() methods that take `MemorySegment`.  (Sorry, I
   admit this point wasn't clear).

 - `getZoneReloader()` interface: I'm not sure if it makes sense to
   pass rrclass to this factory because the zone table segment should
   know that (at least in theory).

 '''zone_reloader_unittest.cc'''

 - Does this test the case where we don't call cleanup() to confirm the
   previous data are still cleaned up?

 '''zone_table_segment_unittest.cc'''

 - s/load_action/loadAction/

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