Opened 8 years ago

Closed 8 years ago

#1330 closed task (complete)

Extend DatabaseAccessor to support reading diffs

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

Description

We need to implement some new methods:

  • getDiffs() (like getRecords) this returns a kind of iterator.
  • add getNext() to this iterator class. It returns the next "diff" in the form of RRset (in fact, essentially an "RR")
  • implement these for SQLite3Accessor

Subtickets

Change History (22)

comment:1 follow-up: Changed 8 years ago by stephen

I think this needs a little more specification. When it says "next "diff" in the form of RRset (in fact, essentially an "RR") ", is this:

  • One RR for every difference, with an indication of whether the RR was added or removed (so if an RR has been modified, it appears twice, once as the old version and once as the new)?
  • An iterator returning a pair of RRs, "first" holding the RR removed (so may be null) and "second" holding the one added (which also may be null).
  • Each RRset in the new version that has changed, with a special indication for where an RRset has been completely removed?
  • Two separate iterators, one for RRs that have been removed and one for the ones that have been added?
  • Does the list of RRs include the SOA record?

comment:2 in reply to: ↑ 1 Changed 8 years ago by jinmei

Replying to stephen:

I think this needs a little more specification. When it says "next "diff" in the form of RRset (in fact, essentially an "RR") ", is this:

  • One RR for every difference, with an indication of whether the RR was added or removed (so if an RR has been modified, it appears twice, once as the old version and once as the new)?

My intent was this one. See also the wiki page on the difference
design: http://bind10.isc.org/wiki/DifferenceDesign
which I hope clarifies this point a bit better.

Using the example shown in the "Top Level Diff Representation" section,
the first call of getting "next diff" returns this RR:

example.com. SOA serial=1

The second call returns this:

a.example.com. A  192.0.2.1

Third returns this:

example.com. SOA serial=2

and so on.

And since this is already IXFR-style sequence, we don't have to
specify whether it's for "add" or "delete".

FWIW, this is largely derived from the BIND 9's journal API.

But I'm not necessarily insisting this particular design.

  • Does the list of RRs include the SOA record?

Yes.

comment:3 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 5

comment:4 Changed 8 years ago by jelte

  • Add Hours to Ticket 5 deleted
  • Estimated Difficulty changed from 0 to 5

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111108

comment:6 Changed 8 years ago by stephen

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

comment:7 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:8 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to UnAssigned

Now ready for review. Suggested ChangeLog entry:

Added iterator to access records in the IXFR "diffs" table.

comment:9 in reply to: ↑ 8 Changed 8 years ago by jinmei

Replying to stephen:

Now ready for review. Suggested ChangeLog entry:

Did you intentionally not move the ticket to the review queue?

comment:10 Changed 8 years ago by stephen

  • Status changed from assigned to reviewing

Did you intentionally not move the ticket to the review queue?

No, my mistake - now in the review queue.

comment:11 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:12 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

side note1: i made a couple of minor style adjustments, see
4ca71b858671d112fade23b449f2a59f14d1d300

side note2: Seeing this interface also reminded me that I wanted to
propose defining a class for serial numbers (with overloaded comparison
operators), instead of passing around plain utin32_t values. Added a
comment to #1278.

I'm not sure I'm comfortable with the exception approach we are taking
here; (defining a number of them in sqlite3_accessor.h and simply
saying on the database.h level it can throw 'any'). Perhaps I'm
overthinking this, but if we want to differentiate between different
errors on our calling side (i.e. in our modules that use these
datasources), then we'd be better off defining a number of 'standard'
exceptions for modules to use. Of course we'd still need to catch
'all' with a generic catch clause, but reporting for 'normal' failures
would probably be more specific then. Not saying that these new errors
would not fall under 'other' that way, but simply saying 'any error
can be raised', while true, might be cutting it a bit short.

The code looks ok, and if you disagree with the above, it can be
merged; only one small code issue in sqlite3_accessor.cc; int rc is
declared twice in getSingleValue(). Compiles fine, but not so nice :)

comment:13 Changed 8 years ago by jinmei

I happened to notice a couple of things whiel I worked on #1332:

  • there's a missing EXTRA_DIST (I've fixed it on the branch and pushed it). Please be sure pulling it before merge.
  • Is it guaranteed that this SQL statement provides what we want?
        "SELECT rrtype, ttl, id, rdata, name FROM diffs "   // DIFF_RECS
            "WHERE zone_id=?1 AND id>=?2 and id<=?3"
    
    That is, wouldn't it be possible that the order of the resulting rows is arbitrary?

comment:14 Changed 8 years ago by jinmei

Another thing: exception should be caught by (const) reference:

        } catch (TooLittleData) {

comment:15 Changed 8 years ago by jinmei

And yet another thing (this one is critical): aren't the OPERATION incorrect?
0 = DIFF_ADD and 1 = DIFF_DELETE, so it seems to retrieve the first
added record of the given begin serial to the last deleted record of the
end serial (which is different from the IXFR ordering).

    "SELECT id FROM diffs "     // LOW_DIFF_ID
        "WHERE zone_id=?1 AND version=?2 and OPERATION=0 "
        "ORDER BY id ASC LIMIT 1",
    "SELECT id FROM diffs "     // HIGH_DIFF_ID
        "WHERE zone_id=?1 AND version=?2 and OPERATION=1 "
        "ORDER BY id DESC LIMIT 1",

Also, we should probably document somewhere that in the SQLite3
accessor we use consistent values for "operation" between the API
and the database schema.

comment:16 Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

side note2: Seeing this interface also reminded me that I wanted to propose defining a class for serial numbers (with overloaded comparison operators), instead of passing around plain utin32_t values. Added a comment to #1278.

Good idea.

I'm not sure I'm comfortable with the exception approach we are taking here; (defining a number of them in sqlite3_accessor.h and simply saying on the database.h level it can throw 'any')

I used the term "throwing any" in header documentation as it was used elsewhere in the code. As to the exception approach:

  • In the short term, the exceptions are now subclasses of DataSourceError, giving a common ancestor class that can be caught through a "catch" clause.
  • In the long-term, I think we want to decide how we deal with errors. In some cases we are using exceptions (with free-form text in the source code), in other places we are using the message system (with defined text in external files and the text listed in a separate messages manual). I think we need some guidelines as to where to use one rather than the other.

only one small code issue in sqlite3_accessor.cc; int rc is declared twice in getSingleValue(). Compiles fine, but not so nice :)

Oversight - removed.

Is it guaranteed that this SQL statement provides what we want?

Good catch - there was an "ORDER BY id ASC" missing from it which has now been added.

Another thing: exception should be caught by (const) reference:

Done.

And yet another thing (this one is critical): aren't the OPERATION incorrect? 0 = DIFF_ADD and 1 = DIFF_DELETE, so it seems to retrieve the first added record of the given begin serial to the last deleted record of the end serial (which is different from the IXFR ordering).

I used the values and the statements from the current version (v10) of the DifferenceDesign page which states that "operation" is either "0 (delete) or 1 (add)".

As database.h defines the DiffOperation enum, I've made the "operation" value in the SQL statements (the ones selecting the index limits) a statement parameter and bind to it at run-time with DIFF_DELETE or DIFF_ADD. The overhead is negligible and it does mean that the SQL string does not contain as literal values numbers defined elsewhere. I've also updated the data used for the unit tests with the changed values. I'll update the DifferenceDesign page when I merge the code.

Other
Finally, does this merit a ChangeLog entry? If so, then how about something along the lines of:

[func]     stephen
Add C++ API for accessing zone difference information in database-based
data sources. 

comment:17 follow-up: Changed 8 years ago by jinmei

I believe NoSuchSerial should be defined at a higher level (actually
the top level) header file.

comment:18 in reply to: ↑ 17 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

Replying to jinmei:

I believe NoSuchSerial should be defined at a higher level (actually
the top level) header file.

toomuch and toolittle should too, IMO. Putting them on the level of database.h would also be fine for me, but we might as well collect them in 1 place (even if some implementations won't use them). SQlite3Error is really specific for sqlite, so that one makes sense here (unless we change it to something more general, but i have no strong opinion about that). As soon as we're going to implement any other 'free data' backend I suspect we'll run into the same error conditions.

For the rest the changes look fine to me, not sure if jinmei has more comments :)

comment:19 Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

I've moved the NoSuchSerial exception to data_source.h (rather than database.h) on the grounds that (theoretically) any data source could hold a differences table.

comment:20 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

so we'll move the others later? :)

Then I think this can be merged.

comment:21 Changed 8 years ago by jinmei

To be clear:

I'm basically just mentioning what I happen to notice rather than acted
as another blocking reviewer.

As long as the critical one (the add/delete semantics) was resolved,
which I know was and Jelte is okay with the branch I have no problem
with merging it.

comment:22 Changed 8 years ago by stephen

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged as commit 120946aa30b22c36995135b7d5bfcade4c26e192

Note: See TracTickets for help on using tickets.