Opened 7 years ago

Closed 7 years ago

#2541 closed task (complete)

add addZone() interface for data source

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20121218
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: loadzone-ng
Estimated Difficulty: 0 Add Hours to Ticket:
Total Hours: 11.5 Internal?: no

Description

To complete the loadzone-ng tool (#2380), we need some way to add a
new zone to the "zones" table of the sqlite3 DB file (or in general
any backend of data sources, but in practice it means sqlite3 for the
moment); otherwise we cannot load a new zone from the scratch.

A tentative proposal to do this is to extend the DataSourceClient
class, adding the new addZone() method:

class DataSourceClient : boost::noncopyable {
public:
    // It first checks if the specified name of the zone exists.  If it
    // exists it returns false; otherwise it adds information of the
    // new zone in backend-dependent manner and returns true.
    // The DB-based version of this method would perform the check and add in
    // a single transaction.
    //
    // Throws on any unexpected failure.
    virtual bool addZone(const dns::Name& zone_name) = 0;
};

And, for the DB-backend data sources, extend the DatabaseAccessor
class:

class DatabaseAccessor : boost::noncopyable {
public:
    // add the specified name of zone, return the zone ID.
    virtual int addZone(const std::string& name) = 0;
};

and implement it for SQLite3Accessor.

We may have to heavily revisit the interface later (in which case some
of the things done in this ticket will become a waste), but it's still
probably better to add ad-hoc SQLite3-specific code to loadzone-ng
itself.

Subtickets

Change History (14)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20121218

comment:2 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:3 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

I ran into a few snags and last-minute design decisions, some of which may be controversial, so if whoever would pick this up for review has any problems with the following, you are allowed to let me know first before you review the rest :)

  • addZone was already a method in MemoryClient? (with a different footprint), and rather than changing the existing one I opted for naming the top-level call 'createZone()'. but we could move the existing one and call the new one addZone of course.
  • Instead of a pure virtual, I made createZone() have a default implementation that throws NotImplemented?
  • The specific implementation for SQLite3Client does not hold a transaction, and does not do duplicate checks, like addRecordToZone i kept it as dumb as possible. (what id it returns in such a scenario is kind of undefined)
  • It is DatabaseClient? that uses startTransaction and commit() to keep the transaction between the lookup and the creation (Added a RAII-style holder class for that (i guess we could update the DatabaseUpdater? class to use this one as well and not have it duplicate code)).

(and now that it works like that, at least at the moment, it is really not necessary for SQLite3Accessor::addZone() to return anything at all, its return value is ignored anyway. However, if we keep it in it is probably easier to make an addZone that directly creates an updater, or have the updater create the zone, should that discussion be picked up again).

comment:4 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:5 follow-up: Changed 7 years ago by jinmei

client.h

  • as for providing the default definition in the base class (and still not making it pure virtual): In general, I've been trying to avoid it so that these default versions are not used accidentally (at the cost of requesting defining something in each derived class). I'd also generally avoid relying on the "not implemented" default, because it's quite likely to suggest that the abstraction of the base class isn't really good. But for this particular case we're doing some kind of hack, which is a bit better than the famous "short term workaround", so, assuming we shouldn't be afraid of totally revisiting it, I'm okay with breaking these two principles. But I'd suggest adding some considerations as a comment related to this. (See also the next bullet for providing the default definition).
  • parameter name doesn't appear in the declaration. To avoid that while also avoiding to have 'unused parameter' warning, we'd need to move the definition to a .cc file.
  • "fully qualified" sounds redundant to me, because a Name object always represents an absolute name. (but this is a minor point)

database.h

  • addZone() declaration: shouldn't this may be will or must or something?
        /// Callers must also start a transaction before calling this method,
        /// implementations may throw DataSourceError if this has not been done.
    
  • createZone() declaration: I'd skip describing param and return if there's nothing special to the database version (to avoid repeating the same thing).

database.cc

  • TransactionHolder destructor: rollback() can throw, so we need some consideration for that case. See also ~DatabaseUpdater().

sqlite3_accessor.h

  • Per style guide, we'd use C++-style comments for doxygen
        /**
         * \brief Add a zone
         *
    
    Maybe you were trying to be consistent with the style for the previous method, but consistency is already broken within the file, and since this branch isn't big, I'd suggest providing consistency throughout the file (by changing others too).
  • For SQLite3 specific text, "empty" looks awkward:
         * This implements the addZone from DatabaseAccessor and adds an (empty)
         * zone into the zones table. If the zone exists already, it is still
    
    because this concept can't be represented in the zones table. I'd simply say "adds a zone" here.

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).
  • I believe these could be SQLITE_STATIC (which is a bit more efficient) in this case:
        proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
        proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
    
    in fact, I think TRANSIENT is a better choice in this case because it's safer and performance isn't important here. But it's probably a good idea to explicitly note that in a comment to prevent someone from trying to unnecessarily "optimize" it later.

sqlite3_accessor_unittest.cc

  • Overall: I guess many if not all of ASSERT_xx can be EXPECT_xxx.
  • I'd check the existence of the same name of zone but of a different RR class doesn't cause confusion
  • If possible I'd like to test the case where either/both of add/getZone fails (maybe possible with another write).

database_unittest.cc

  • I'd like to test the case where rollback happens.

comment:6 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:7 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

client.h

  • as for providing the default definition in the base class (and still not making it pure virtual): In general, I've been trying to avoid it so that these default versions are not used accidentally (at the cost of requesting defining something in each derived class). I'd also generally avoid relying on the "not implemented" default, because it's quite likely to suggest that the abstraction of the base class isn't really good. But for this particular case we're doing some kind of hack, which is a bit better than the famous "short term workaround", so, assuming we shouldn't be afraid of totally revisiting it, I'm okay with breaking these two principles. But I'd suggest adding some considerations as a comment related to this. (See also the next bullet for providing the default definition).

ok, added an extra line to the \note. TBH, (ignoring whether or not it is clean design on a higher level), if the default implementation is only the throwing of an exception, that is specifically intended for this scenario, I don't think it too bad.

  • parameter name doesn't appear in the declaration. To avoid that while also avoiding to have 'unused parameter' warning, we'd need to move the definition to a .cc file.

yeah we didn't have one so i just did what the rest of the file did. But good point (one of the others has void casts for this). Added a client.cc file and move the three default implementations there.

(noting that the documented reason for getIterator() to have a default notimplemented is different)

  • "fully qualified" sounds redundant to me, because a Name object always represents an absolute name. (but this is a minor point)

That I actually did on purpose; but more to be very clear that there isn't some 'base name' that will be appended (and FQDN tends to be more clear than 'complete' or something). But also no strong opinion :)

database.h

  • addZone() declaration: shouldn't this may be will or must or something?
        /// Callers must also start a transaction before calling this method,
        /// implementations may throw DataSourceError if this has not been done.
    

'should' i guess, though in this case it is tricky to find the right wording (it depends on whether you are reading it as an implementor of a datasource or as a user of the API)

  • createZone() declaration: I'd skip describing param and return if there's nothing special to the database version (to avoid repeating the same thing).

ok

database.cc

  • TransactionHolder destructor: rollback() can throw, so we need some consideration for that case. See also ~DatabaseUpdater().

oh of course. Added, similar log description as in the updater case)

sqlite3_accessor.h

  • Per style guide, we'd use C++-style comments for doxygen
        /**
         * \brief Add a zone
         *
    
    Maybe you were trying to be consistent with the style for the previous method, but consistency is already broken within the file, and since this branch isn't big, I'd suggest providing consistency throughout the file (by changing others too).

I was, but sure, done :)

  • For SQLite3 specific text, "empty" looks awkward:
         * This implements the addZone from DatabaseAccessor and adds an (empty)
         * zone into the zones table. If the zone exists already, it is still
    
    because this concept can't be represented in the zones table. I'd simply say "adds a zone" here.

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 believe these could be SQLITE_STATIC (which is a bit more efficient) in this case:
        proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
        proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
    
    in fact, I think TRANSIENT is a better choice in this case because it's safer and performance isn't important here. But it's probably a good idea to explicitly note that in a comment to prevent someone from trying to unnecessarily "optimize" it later.

ok

sqlite3_accessor_unittest.cc

  • Overall: I guess many if not all of ASSERT_xx can be EXPECT_xxx.

I tend to prefer to have the first of 'a set' be an assert (as in, if that one fails, it is highly likely the rest will too anyway). But indeed some could as well be expect.

  • I'd check the existence of the same name of zone but of a different RR class doesn't cause confusion

added

  • If possible I'd like to test the case where either/both of add/getZone fails (maybe possible with another write).

Added a test that'll make add fail, but not sure how to do it for getZone (after add succeeded)

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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by jinmei

I made a few additional minor changes. Please pull and check them.

Replying to jelte:

ok, added an extra line to the \note. TBH, (ignoring whether or not it is clean design on a higher level), if the default implementation is only the throwing of an exception, that is specifically intended for this scenario, I don't think it too bad.

My point is that if a base class needs to have a default
implementation of a virtual method that throws a "not implemented"
exception, it means the class has too much responsibility because the
fact that it's the default means most of the derived classes wouldn't
support the feature that the method originally intends to do (not
throwing the exception). Design-wise, if we find such is a case, what
we should do to revisit the class design rather than to add a top
level method that wouldn't be supported by most of actual derived
classes. But that's a general topic. I'm not requesting a change to
this branch due to this point.

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.

  • If possible I'd like to test the case where either/both of add/getZone fails (maybe possible with another write).

Added a test that'll make add fail, but not sure how to do it for getZone (after add succeeded)

For getZone() it's probably impossible to do in a reasonable way.

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.
  • If we need to use the direct SQLite3 operation, I'd like to make sure db is closed at the end of the test, no matter how it ends. So we need some guard/holder object to guarantee the cleanup.
  • do you mean "locked" here?
        // addZone should throw exception that it is locket
    

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.

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.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:10 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to 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?
        // 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 :)

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jinmei

Replying to jelte:

I made a few additional minor changes. Please pull and check them.

I made yet another cleanup commit.

database_unittest.cc

  • I'd like to test the case where rollback happens.

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

Yes.

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.

The added test (createZoneRollbackOnLocked) is fine. By this we can
confirm (that it results in exception and) exception safety of
createZone(), which cannot be tested in other scenarios. I don't see
strong need for createZoneRollbackOnExists(), but I'm okay with having
it too.

One final comment:

sqlite3_accessor_unittest.cc

Shouldn't rollback be done before the final check?

    // New zone should not exist
    EXPECT_FALSE(accessor->getZone(new_zone).first);

    accessor->rollback();

I suspect even if another_accessor succeeded to add the zone
(e.g. with backend support for more fine-grained lock) the other
accessor would still not see the new zone unless it exits from the
transaction.

If I'm correct on this, please make the trivial fix and merge.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:13 Changed 7 years ago by jinmei

  • Add Hours to Ticket changed from 0 to 3.5

comment:14 in reply to: ↑ 11 Changed 7 years ago by jelte

  • Add Hours to Ticket 3.5 deleted
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 11.5

Replying to jinmei:

Replying to jelte:

I made a few additional minor changes. Please pull and check them.

I made yet another cleanup commit.

ah duh, thanks :)

One final comment:

sqlite3_accessor_unittest.cc

Shouldn't rollback be done before the final check?

    // New zone should not exist
    EXPECT_FALSE(accessor->getZone(new_zone).first);

    accessor->rollback();

I suspect even if another_accessor succeeded to add the zone
(e.g. with backend support for more fine-grained lock) the other
accessor would still not see the new zone unless it exits from the
transaction.

If I'm correct on this, please make the trivial fix and merge.

yeah, done.

Thanks! merged, closing ticket.

Note: See TracTickets for help on using tickets.