Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1329 closed task (complete)

Extend DatabaseAccessor to support adding diffs

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20111108
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: 7:01 Internal?: no

Description

We need to implement a new method: addDiff(). It takes string-based
information for the diff, and update the internal database (depending
on details of derived accessors) accordingly.

Aslo implement it for SQLite3Accessor.

Subtickets

Change History (15)

comment:1 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 5

comment:2 Changed 8 years ago by jelte

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

comment:3 Changed 8 years ago by jelte

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

comment:4 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:5 Changed 8 years ago by jinmei

trac1329 is ready for review.

This is basically a straightforward implementation of the
addRecordDiff method as described in http://bind10.isc.org/wiki/DifferenceDesign
with detailed tests and considerations for minor/error cases, so I
believe it's easy to review.

One possible point that would require an attention (and perhaps
discussion) is commit 3d59d6a. In this commit I changed the way we
prepare SQLite3 statements: they are now prepared first time they
are needed rather than at the time of open(). The primary motivation
is compatibility: since this task introduces a new table, an attempt
of preparing the statement for adding diffs on an older version of
database will fail, even if diff handling is not required in that
environment. I could bump the schema version and automatically
update the schema at the initialization time, but I didn't want to
tweak the accessor class in this direction further. IMO we should
eventually (soon) introduce a separate framework to manage data
sources at a higher level, and this issue should be solved there.

This change has other advantage: in many cases a single accessor
object will only need a subset of the entire statements, so delaying
the setup will help improve performance (although probably
marginally).

The size of this commit is not small, but I believe this is basically
straightforward.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:7 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to jinmei

(very general note; we seem to be pretty inconsistent in using /// and
/** notation for doxygen comments, even within single files. Certainly
unrelated to this ticket, but perhaps we want to make this a bit more
consistent. Especially for instance in database.h where the comment
for DiffOperation? uses ///, the code above and below it use /**, and
at some point everything is /// again)

I like the getStatement() approach, btw.

Should we have some way of signaling whether journaling is supported
by the datasource at all?

Just one real comment, in sqlite3_accessor.cc, and about a (probably)
temporary method anyway. So if we plan to ditch it, maybe we should
only add some comments in the cc file regarding the potential issues
below (in case it is taken as the base for whatever we write later).
Or you can ignore the comments below here :)

SQLite3Accessor::getRecordDiff()

The return value of sqlite3_step should probably be checked for more
than SQLITE_ROW or not (I am mainly thinking of SQLITE_ERROR). Not
sure whether it should then drop the result and raise an error or keep
the current behaviour of just returning what it found so far, but I am
leaning towards the first (as one might get unexpected behaviour this
way should something really bad be going on)

BTW, I may be premature optimizing things here, but that interface of
getRecordDiff could be more efficient if we pass it a vector instead
of letting it return one. Unless of course we assume this is optimized
away by the compiler. A drawback would be that it wouldn't be
exception-safe on this level if the previous comment is taken into
account, but handling that may be a caller-level issue. I am noting it
because interfaces are much harder to change than code itself :)

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

Replying to jelte:

(very general note; we seem to be pretty inconsistent in using /// and
/** notation for doxygen comments, even within single files. Certainly
unrelated to this ticket, but perhaps we want to make this a bit more
consistent. Especially for instance in database.h where the comment
for DiffOperation? uses ///, the code above and below it use /**, and
at some point everything is /// again)

Yeah, I know we mix the two styles. I personally would like to make
them more consistent, but has been ignoring this point as it might be
a pure bikeshed. (Note also that we allow both styles in our coding
guideline). But it may make sense to introduce some guideline at
least inside each single file and/or for doxygen comments. Maybe
discuss this at the next biweekly call?

For this particular case, I've changed the style for DiffOperation to
make it consistent at least locally.

I like the getStatement() approach, btw.

Okay, good.

Should we have some way of signaling whether journaling is supported
by the datasource at all?

I think we should. I'm not 100% sure what is the best way to do this.
Unlike the case for updater and iterator we cannot signal this at the
construction time. I'd be adding a general interface to the
DataSourceClient class to return specific capabilities (writable,
iteratable, journaling, and perhaps more) of the underlying data
source. xfrin or dynamic update server can check this before creating
an updater, and specify journaling only when it's supported.
(Should we create a ticket for this?)

Whether or not do this, it would also make sense to allow the
implementation of addRecordDiff to signal it when journaling isn't
supported. So I updated the documentation saying it will throw
NotImplemented in such a case (no test for this because it's
supported SQLite3).

Just one real comment, in sqlite3_accessor.cc, and about a (probably)
temporary method anyway. So if we plan to ditch it, maybe we should
only add some comments in the cc file regarding the potential issues
below (in case it is taken as the base for whatever we write later).
Or you can ignore the comments below here :)

You're right, and you also rightly guessed that it was intended to be
ditched. So I'd leave the code as is, but add some more comments
about the intent.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

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

Replying to jinmei:

Replying to jelte:

(very general note; we seem to be pretty inconsistent in using /// and
/** notation for doxygen comments, even within single files. Certainly
unrelated to this ticket, but perhaps we want to make this a bit more
consistent. Especially for instance in database.h where the comment
for DiffOperation? uses ///, the code above and below it use /**, and
at some point everything is /// again)

Yeah, I know we mix the two styles. I personally would like to make
them more consistent, but has been ignoring this point as it might be
a pure bikeshed. (Note also that we allow both styles in our coding
guideline). But it may make sense to introduce some guideline at
least inside each single file and/or for doxygen comments. Maybe
discuss this at the next biweekly call?

ok, i'll mention it to shane

Should we have some way of signaling whether journaling is supported
by the datasource at all?

I think we should. I'm not 100% sure what is the best way to do this.
Unlike the case for updater and iterator we cannot signal this at the
construction time. I'd be adding a general interface to the
DataSourceClient class to return specific capabilities (writable,
iteratable, journaling, and perhaps more) of the underlying data
source. xfrin or dynamic update server can check this before creating
an updater, and specify journaling only when it's supported.
(Should we create a ticket for this?)

yeah, created ticket #1382 for this

Whether or not do this, it would also make sense to allow the
implementation of addRecordDiff to signal it when journaling isn't
supported. So I updated the documentation saying it will throw
NotImplemented in such a case (no test for this because it's
supported SQLite3).

ok, cool

Just one real comment, in sqlite3_accessor.cc, and about a (probably)
temporary method anyway. So if we plan to ditch it, maybe we should
only add some comments in the cc file regarding the potential issues
below (in case it is taken as the base for whatever we write later).
Or you can ignore the comments below here :)

You're right, and you also rightly guessed that it was intended to be
ditched. So I'd leave the code as is, but add some more comments
about the intent.

ok.

All good, please merge :)

comment:12 Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

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

Replying to jelte:

All good, please merge :)

Okay, thanks, merge done, closing ticket.

comment:14 Changed 8 years ago by jinmei

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

comment:15 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 7:01
Note: See TracTickets for help on using tickets.