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:14 jinmei]:
 > 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.

 It is indeed plus, but you exchange it for something far more complicated
 on
 the caller side than keeping the order right (with quite logical names).

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

 I'm not aware of me adding them, but I might have short memory. Will you
 create the clean-up ticket, please?

 > 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:
 >
 > [...]
 >
 > Can't we do something like this?

 I did that. It just seems more natural for me to do it the separate
 variables way, I don't know why, so I usually do that first.

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

 Ah, OK. I added that.

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

 I asked him if he could do it on merge.

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

 It won't work exactly as you describe, because in the second attempt to
 load, it complains and won't even call the callback. But I tried something
 in that direction.

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