BIND 10 #963: Utility to upgrade SQLite schema

[BIND 10 Development]

#963: Utility to upgrade SQLite schema
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  stephen
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:         |  Sprint-20120320
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  sqlite schema upgrade              |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => stephen


Comment:

 Hello

 I have several questions.

 '''Some behaviour questions'''

 Is the goal to upgrade any kind of database, or only SQLite3? If the
 second, would it be better named like `b10-sqlite3util`?

 While it might be harder to implement, I'd rather take the default
 database from configuration, if possible. Or, to avoid confusion, have the
 user always enter the filename of the database (otherwise the user might
 have configured a different database, forgot about it and this one says
 the default is OK, but the server does not work).

 Is there any reason to limit the upgrade utility to one database at a
 time? I believe we support (or, if not now, will support) multiple SQLite3
 database files to serve from, it would make sense to just list them all at
 once.

 Similarly to utilities like `fsck`, would it make sense to exit with known
 non-zero exit code if the check says the database is not up to date? This
 would make it easier to use in automated scripts.

 Related to this ‒ while it would be out of the scope of this ticket, I'd
 like the boss to be able to run a component in „blocking mode“ ‒ the
 component is run, it waits for it to exit and an exit code is checked.
 Then we could run the `--check` (possibly with something like `--connect-
 to-msgq`) mode on startup and fail early before starting everything or
 warn or anything. To do this, we need few changes:
  * Log through our logging system (I think this could be done now, and the
 --verbose flag could be handled by it). This will also eliminate some of
 the code (like the functions `output` and friends).
  * Connect to the message queue to read configuration (of logging and data
 sources to read the files from). This I think is out of the scope of the
 ticket.

 '''Tests'''

 Is there any actual reason to implement it in shell? I don't say it is
 wrong, but it is unusual and inconsistent with the rest, so there should
 be a reason for it.

 Is there any reason to use doxygen for the shell code? I don't think
 doxygen extracts it (it doesn't do now for sure, because the directory is
 not listed, but I don't think it even can) and it clutters the comments
 when read plain-text.

 I don't like this construction with `x`. I don't know where the idiom
 comes from, but in case any of the variables contain spaces, this might
 even be wrong:
 {{{
 if [ x$expected_schema = x$actual_schema ]
 }}}

 Is there any reason at all not to use the AFAIK more correct and more
 intuitive construct?:
 {{{
 if [ "$expected_schema" = "$actual_schema" ]
 }}}

 Also, these tests don't check separate functions (well, they hardly can
 from shell), so they should be considered system-level tests, not unit
 tests. I believe they should be moved there, then.

 Does the test file need to be generated from `.in`? I didn't see any
 replacement performed.

 I believe some files are missing from `EXTRA_DIST`. The tests fail in
 distcheck (but pass in check), with bunch of errors like:
 {{{
 --- PASS
 ERROR: database file
 /home/vorner/work/bind10/bind10-devel-20120316/_build/../src/bin/dbutil/tests/testdata/no_schema.sqlite3
 not found
 *** FAIL
 }}}

 '''Documentation'''

 Did we already decided to use doxygen for python? And, why use the `@`
 sign if all the rest of the code uses `\`?

 Even classes can have the {{{"""string"""}}} documentation comment.
 Therefore for completeness and consistency, should the `Database` class
 should be documented that way, not in a comment before the class.

 If it is all doxygen, then we should not miss:
  * `upgrades`
  * `DbutilException`

 About the user-level documentation, I believe we need at least little bit
 of it. Otherwise, are the users expected to guess to use it? Unless they
 can really use it, I don't see a reason to ship it.

 Also, what will happen after the major version upgrade now? Will the
 current code reject it? I think as part of merging this, we should upgrade
 the version the code expects.

 '''Code'''

 When deciding the name for the backup, there's a race condition. Is that
 OK?

 I believe that better than checking for some errors (like missing a line
 in the version table) and omitting all the rest, the `get_version`
 function could be better implemented using `try` ‒ `catch` (simply try
 getting the version in the most straight-forward way, it will raise some
 exception if it fails, be it problem with opening the database or indexing
 beyond the end of array when there's no line in the table).

 This error is not really very helpful I think:
 {{{#!python
             raise DbutilException("database not at latest version but no "
 +
                                   "upgrade was performed")
 }}}
 Maybe something like „Database not at latest version, but I don't know how
 to upgrade. Either your database is bogus or the utility is missing an
 upgrade scenario.“ ‒ possibly with listing the version that the database
 is at.

 Thanks

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