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

 * owner:  vorner => jinmei


Comment:

 Hello

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

 I'm not sure about this. It seems to make the use and the interface of the
 class more complex. I don't know if we need the flexibility either, we
 everything that's in the critical section should happen at once. And
 there's
 some preparation before and some cleanup after. With the fact that any of
 them
 can simply be an empty method, I think we have all the flexibility we
 need.

 Maybe we could rename the `load()` to something like `prepare()`. But I
 kind of
 don't see any advantage in your proposal and it makes the code more
 complex.

 > - this is not in the guideline, but I though we have general agreement
 >   about adding a blank line before the `\brief` comment line:

 It's not that I wouldn't agree, I just tend to forget O:-).

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

 I tried that. But I don't see any advantage in that either. In theory, it
 could
 work with any other implementation of the class.

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

 Um, is there a problem with the `getHeader().getTable()` that's being used
 in
 the `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.

 In that case, there's a confusion about `Unexpected`. Grep through the
 `memory/` directory for it and you'll see many occurrences of
 `Unexpected`, and none of them looks like a system error.

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

 Well, the `install()` „eats“ the data, so `data_ready_` becomes false
 there (so
 the old version of the zone data won't get installed again by accident).
 But
 `loaded_` stays until the end of life of the class, so we do not try to
 override data we have there (either new or old). So these two booleans
 looked
 like an easy way to deal with it. Maybe there could be an enum showing in
 which
 step we are, but I'm not sure if that would make it any more readable.

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

 The zone table is not being created there. This only gets the pointer to
 the existing one, so it can be used more conveniently in the following
 code:
 {{{#!c++
 ZoneTable* table(segment_->getHeader().getTable());
 }}}

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

 The base class says:
 {{{#!c++
 /// \return New instance of a zone reloader. The ownership is passed
 ///     onto the caller.
 }}}

 Isn't that explicit enough?

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

 That's why I added the `setZoneTable()` as a temporary interface only. I
 expect
 the creation will be implemented in some ticket soon, in which point
 there'll
 be no more need for the `setZoneTable()` method. Or should it be
 implemented in
 this ticket too?

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

 Well, but in practice, it seems not to know. So I can either pass it
 through
 the parameter in this branch or try fixing classes around, but that feels
 like
 out of scope of the ticket.

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