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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:17 jinmei]:
 > 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.

 ACK

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

 Yes, I see. I didn't notice that before, so let's keep it like it is.

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

 I should try to remember the reason.

 Anyway, this part is OK to be merged (or you can wait for the rest to
 finish being reviewed).

 Replying to [comment:18 jinmei]:
 > - use gtest's TYPED_TEST to share most of the test cases.  Due to the
 >   characteristics of the TYPED_TEST (see the gtest documentation) we
 >   need to explicitly add "this->" to test class member variables.
 >   This is one main reason for the bigger changes.  Some tests only
 >   work with the mock accessor, so I added some workaround to do tests
 >   selectively depending on the accessor type.

 It looks OK. But I'm thinking about one thing ‒ there are several tests
 that are skipped with different types than the MockAccessor (like
 nullIteratorContext). Should we take them out of the typed tests and do
 something like this? So they wouldn't show up in the outputs when they are
 effectively empty.

 {{{#!C++
 typedef DatabaseClientTest<MockAccessor> MockDatabaseTest;

 TYPE_F(MockDatabaseTest, nullIteratorContext) {

 }
 }}}

 With regards

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