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:13 vorner]:
 > Hello
 >
 > Replying to [comment:11 jinmei]:
 > > - on the interface design: maybe just expose the same single
 > >   method, like doNextStep(), instead of load()/install()/cleanup()?
 >
 > 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.

 I personally think that it's a plus that the caller doesn't have to
 care out method call ordering, but I don't push the idea further.

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

 Right now, maybe.  But I expect if it's other types of
 `ZoneTableSegment` (which uses other types of `MemorySegment`) it will
 have to care about some subtle points due to the difference between
 segment types.  Then we need to introduce case-analysis based on the
 actual derived type of `ZoneTableSegment` within `ZoneWriterLocal`.
 On the other hand, if and when we can really be sure it's segment-type
 independent, I think we should rather make the entire `ZoneWriter`
 class independent from the segment type and make it work in a
 polymorphic way in terms of `ZoneTableSegment`.

 > > - 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()`?

 Ah, right.  I guess I was somehow confused here (and I certainly
 misunderstood 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.

 I guess many of you were added by yourself:-) but in any case I admit
 it hasn't been officially clarified which exception (especially
 `Unexpected`) should be when.  We'll probably need a cleanup (and
 documentation) ticket.

 BTW, I didn't mean it's *only* for system errors.  I just meant a
 system error is one of the intended cases for this exception.  See
 below also.

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

 After drawing a state transition diagram with tracking all these
 `loaded_`, `data_ready_` and `zone_data_`, spending some amount of
 time, I see the need for these and have some sense the code is
 correct.  But to be honest I cannot be 100% sure about the correctness
 because of the combination of these variables.

 I'd probably introduce a single state variable, whose value can be
 (maybe INIT and) LOADED, INSTALLED, CLEANED_UP, and only allow the
 following transitions:
 - INIT->LOADED->INSTALLED->CLEANED_UP(->destroyed)
 - INIT->LOADED->CLEANED_UP(->destroyed)
 - INIT->CLEANED_UP(->destroyed)
 - INIT->(->destroyed)

 It's less flexible about possible transitions (the current
 implementation allows INIT->CLEANED_UP->LOADED), but should
 cover all practical cases, and would at least reduce one variable to
 track.

 Can't we do something like this?

 (BTW, IMO such issues should be mitigated if we only have doNextStep()
 method and controls state transitions completely within the class).

 > > - `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());
 > }}}

 Yeah, sorry, I misunderstood the code at the time of review.  Please
 just forget this point.

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

 Maybe it's enough in practice, but I'd personally be more explicit,
 like:
 {{{#!cpp
     /// \return New instance of a zone write. The ownership is passed
     ///     onto the caller, and the caller needs to \c delete it when
     ///     it's done with the writer.
 }}}
 because the information on the ownership doesn't necessarily specify
 how the object was allocated (and should be destroyed).

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

 Hmm, in retrospect this should have been done in #2206.  I guess
 Mukund is doing it in #2208 - please confirm that with him.  If #2208
 is adding it, please just go with `setTable()` for now, and make sure
 it will be removed once #2208 is merged; if not, please discuss it
 with him and make sure either #2206 or #2208 does it.

 Comments on the revised version follow:

 '''zone_write.h'''

 - As for the strong guarantee, I'd suggest noting it in the
   documentation, e.g.:
 {{{#!cpp
 /// Each derived class implementation must provide the strong exception
 /// guarantee for each public method.  That is, when any of the methods
 /// throws, the entire in-memory data should roll back to the state at the
 /// time of the call to that method (how to implement it can be
 implementation
 /// dependent).
 class ZoneWriter {
 }}}
 '''zone_write_local.cc'''
 - `ZoneWriterLocal` destructor: I believe we can now remove "Or should
   we assert instead?"
 {{{#!cpp
     // Clean up everything there might be left if someone forgot, just
     // in case. Or should we assert instead?
 }}}
 - `ZoneWriterLocal::install`: actually, in the second throw would be a
   good example where `Unexpected` is better.  This case should most
   likely be an internal bug (not caller's mis-operation), and we could
   probably even assert() it.  But the point of preparing the zone
   table within the segment is far from this class, using an exception
   may be a better choice here, and, in that case, that would really be
   an "unexpected" error.

 '''zone_write_unittest.cc'''

 - the retry test: I think it's a bit weak for testing the strong
   guarantee.  I'd first load some content to the segment, then trying
   to load something else with load_throw_ being true, and then confirm
   the original data is intact.

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