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-20110927
                  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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:15 vorner]:

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

 I was aware of the need (and the lack in the current implementation)
 for adding/removing zones.  I also think we should move the
 functionality of creating a new DB outside the basic responsibility of
 the accessor (in fact it does something beyond the scope of its
 work).  But as you thought I think these extensions/cleanups would
 better be deferred to later plans.

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

 Ah, good catch.  Yes, it should be "differ".  Fixed.

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

 It's a subjective decision (or preference).  I wanted to keep the size
 of the class definition reasonably concise so that the entire
 definition can be browsed at a glance (e.g., without scrolling on a
 reasonably larger size of window).  So I moved some relatively larger
 methods outside the definition.  On the other hand I admit it may look
 inconsistent as some others are still in the class definition even if
 they are super simple ones like getFinder(), and the conciseness
 argument may not be that convincing anyway if we want to provide more
 inline comments.  So this is not a strong preference.

 >  * Should addRRset take the attached signature into account as well?
 (eg. issue the th addRRset on the signature if it's present, or something)

 May or may not be.  As we briefly talked before in a different context,
 I personally wanted to revisit the design of how to represent signed
 RRsets, and would hesitate to make the interface "too convenient"
 until then (including the possible conclusion of not changing the
 design).

 As far as I can see we shouldn't need to allow this mode in the
 meantime, so I believe we can safely defer it to later discussions.
 But to prevent accidental misuse I added explicit checks so that
 an attempt of adding/deleting RRset + RRSIG RRset will result in an
 exception (for the delete case it would actually make sense to
 prohibit it).

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

 Ah, good point.  As for why as member variables, they were originally
 vectors, and I wanted to avoid the overhead of constructing them
 every time the methods are called (but even then it might be a bad
 attempt of premature optimization - after all these methods are not
 expected to be performed in a very performance sensitive path).  Now
 they are plain arrays, it makes even more sense to define them locally
 in the methods.

 As for SIGTYPE, yes, you're right, the original code had a bug.

 I updated the code to define the arrays as method local, and (although
 it's now pretty obvious to be safe from the code) added a test case
 that would have identified the bug in the previous version.

 >  * 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()}}}?

 Hmm, good point.  I'd call it a bug if the destructor of an accessor
 ever threw (it would cause other serious effects even if not here),
 but since the accessor API is public and we expect external developers
 to write their own accessors possibly with bugs, it would be better to
 be as proactive as possible.  So I changed the code as suggested.

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

 I have no strong opinion on for/against the consistency per se, but it
 wouldn't be that straightforward in this case.  According to the
 current API design we need to return a NULL pointer if the specified
 zone doesn't exist.  This condition is checked via the return value
 from startUpdateZone(), so if we included the call in the
 DatabaseUpdater constructor, we'd need to have it throw for
 non-existent zone, catch it in getUpdater (and differentiate it from
 other errors resulting in the same exception type), and convert it to
 a NULL pointer.

 Since the DatabaseUpdater() class details are purely internal to the
 database.cc implementation, I think we can be a bit more flexible on
 the consistency on the interfaces.  So, right now I've not modified
 the code on this point, preferring implementation simplicity.

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

 Possibly, or possibly not.  Right now it's not clear how we relate the
 in-memory client to its underlying databases (or some other backend
 storage) and how applications that need to get access to the
 underlying storage directly (e.g. whether it's via the in-memory
 interface or not).

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

 I think we discussed this before - in the current implementation,
 that's true, but I personally prefer making this explicit.  IMO it's
 fragile to rely on the existence of a pure virtual method (without a
 definition) for this purpose; we may want to revise the interface to
 provide the default implementation at the base class level.  We could
 still provide the default with keeping it pure virtual by defining the
 pure virtual function and having derived classes call it explicitly,
 but we are not currently using that practice.  Also, while the same
 argument could apply to the constructor (we might want to make it
 public for some reason), I believe it's much less common in practice.

 If you still don't like the redundancy in explicitly declaring and
 defining the constructor or do want to have a unified policy, I'm okay
 with having a discussion project wide.  Right now I'm keeping the
 current code.

 >  * Would it help to have a test for „bigger“ transaction (like more adds
 and deletes during one transaction)?

 Good idea.  Added a new test named compoundUpdate.

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