BIND 10 #1068: support writability in the new data source API

[BIND 10 Development]

#1068: support writability in the new data source API
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110830
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  7.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei
 * status:  accepted => reviewing


Comment:

 Hello

 First, notes about the interface ‒ I think it might be useful to be able
 to add and remove zones in backend-agnostic way (and maybe as an atomic
 operation with filling it with data). For example b10-loadzone could use
 it. But it might be out of the scope of this ticket.

 Then, of course, few notes about the code:
  * {{{The underlying realization of a "transaction" will defer for
 different}}} ‒ you mean „differ“ or that we defer it to the different
 implementation?
  * This isn't something that would be wrong in any way and it is possibly
 only style matter, but I'm just curious about the rationale about it ‒ the
 DatabaseUpdater's methods (addRRset, deleteRRset, commit) live outside the
 class definition. Why is it so? I think that the original reason for being
 able to have method implementation outside the class exists because of
 class lives in header file and the code shouldn't, but that doesn't apply
 as both are in the source in this case.
  * Should addRRset take the attached signature into account as well? (eg.
 issue the th addRRset on the signature if it's present, or something)
  * Why are add_columns_ and del_params_ object-level variables? It would
 be enough for them to live only inside the methods they are used in,
 wouldn't it?
  * Related to this: In case the RRSIG isn't the last thing added through
 this updater, any subsequent RRs will have the same ADD_SIGTYPE column.
 Should it be reset afterwards to empty string?
  * Can the destructor of the accessor throw? If so, the commit would have
 a problem ‒ the committed_ would stay as false and the class will call
 rollback on the (already destroyed or partly destroyed) accessor. Should
 the {{{committed_ = true}}} be moved above the {{{accessor_.reset()}}}?
  * A style issue ‒ the commit and rollback are handled from within the
 updater. Should startUpdateZone be called from the DatabaseUpdater's
 constructor instead of outside the class, because of consistency?
  * This is probably out of the scope of this ticket, but shouldn't the
 getUpdater of in-memory propagate the call to the underlying data source
 instead of calling NotImplemented in the long term? If so, maybe there
 should be a note/comment about it?
  * The protected ZoneUpdater constructor is not needed. If anybody tries
 to create instance of it, the compilation will fail because it has pure
 virtual methods.
  * Would it help to have a test for „bigger“ transaction (like more adds
 and deletes during one transaction)?

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