BIND 10 #2541: add addZone() interface for data source

[BIND 10 Development]

#2541: add addZone() interface for data source
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:8 jinmei]:
 > I made a few additional minor changes.  Please pull and check them.
 >

 ack, look ok

 > > > '''sqlite3_accessor.cc'''
 > > >
 > > > - If we require a transaction in addZone(), I think something like
 > > >   `InvalidOperation` is a more appropriate exception for the
 > > >   violation (although I've noticed there are other similar cases in
 > > >   this file where the same argument could apply).
 > > >
 > >
 > > Yeah, I'm willing to change this, but I had originally kept it
 consistent with the other transaction errors.
 > > I also noted a few DataSourceError throws that should probably be
 SQLite3Error btw. Shall I change these as well or do we want to defer
 that? (on a slightly higher level, when doing 2542, I noticed we may want
 a bit of a rearrangement on the client/databaseclient level as well,
 DataSourceError is too overloaded right now IMO)
 >
 > I suggest creating a ticket for this work.  It will be beyond the
 > scope of this task and may cause unexpected regressions that require
 > more time to fix.
 >

 ok done, created #2555

 >
 > As for the` addZoneWhileLocked` test:
 >
 > - I wonder whether we can't do this with another accessor (see also
 >   below).  I thought there are similar tests for updates.

 Ah, yes, I had tried with a simple startTransaction but didn't realize
 that doesn't lock the db just yet. Moved the test to the other fixture for
 more easier setup of startUpdateZone, and doing the test there

 > - do you mean "locked" here?
 > {{{#!cpp
 >     // addZone should throw exception that it is locket
 > }}}
 >

 yup

 > > > '''database_unittest.cc'''
 > > >
 > > > - I'd like to test the case where rollback happens.
 > >
 > > In a way the case is handled (at least, rollback is called in case the
 zone exists already), but it is not checked
 > >
 > > Apart from duplicating the original source code in the NopClient (in
 which case we're not really testing anything) I'm not entirely sure how to
 incorporate that in these tests.
 >
 > Can't we do this with (e.g.) SQLite3 and updater?  See also the
 > previous point.
 >

 Let's see, start an update, try a createZone() (should fail and roll
 back), then roll back the update and do createZone() again (should not
 fail)?

 Added such a test, it still only tests the 'higher level' (i.e. if
 startTransaction() fails itself this test wouldn't notice it, but at least
 it'll show that after a failure it can still add zones.

 Added a second test for the other case as well (add existing zone->fail,
 add new zone->succeed)

 > And, on the revised version of the branch:
 >
 > '''database.cc'''
 >
 > - `TransactionHolder` destructor: the added comment seem to be a mere
 >   copy of the one for `~DatabaseUpdater()`.  According to DRY I'd keep
 >   only one of them, having the other just refer to it.
 >

 ok, referring to the new one in the existing one (the new one is earlier
 in the source code). According to DRY we should probably use the new
 TransactionHolder as a member of the updater btw, instead of repeating the
 catch itself, but the logging would be less precise :)

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