BIND 10 #1061: introduce DatabaseClient (or DatabaseDataSourceClient)

[BIND 10 Development]

#1061: introduce DatabaseClient (or DatabaseDataSourceClient)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110816
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => stephen


Comment:

 Hello

 Replying to [comment:7 stephen]:
 > '''src/lib/datasrc/database.cc'''
 > When will the "TODO Implement" items get done - have tickets been
 created for them?

 That should be the goal of #1162, I needed to have the methods or it
 complained the class had pure virtual methods.

 > '''src/lib/datasrc/database.h'''
 > Retrieving a zone identifier (getZone()) does not seem like
 functionality that should belong to a database connection abstraction - it
 more logically belongs to the Finder class.

 It doesn't. For one, ZoneFinder represents a single zone, so the zone must
 be looked up (and checked it exists) before it is created.

 For another, the current getZone is the lowest possible manipulation with
 the DB, like `select id from zones whene name =`. It can't be done in DB-
 independant way. Maybe the name could be different?

 > __class DatabaseClient__
 > I'm slightly uneasy about passing an auto_ptr as an argument.  True the
 header does correctly state that ownership is transferred on the call, but
 using an auto_ptr does seem an unnecessary complication.  Within the
 class, the same reservation about auto_ptr applies.  An alternative for
 storing the connection is boost::scoped_ptr

 I changed it to shared pointer. But scoped pointer doesn't seem correct,
 because the object is created in some function and must survive
 termination of the function.

 > Exception information should be described using the \exception tag.

 I have nothing against it, but it doesn't seem we use that convention
 much.

 > In the header for findZone, it says "Applications should not rely on the
 specific class being returned, though.".  What does this mean - surely
 applications get a !FindResult" object returned in all paths through the
 code?

 No, I mean it does return DatabaseClient::Finder, but application
 shouldn't just take it and cast it to it. Should I reword it somehow?

 > __class DatabaseClient::Finder__
 > getOrigin(), getClass() and find() do not have header comments.

 Do they need one? They are just implementation of ZoneFinder methods,
 having the same properties. They are not new methods, just new bodies. And
 the user of the DatabaseClient shouldn't be interested in the class anyway
 (as noted), it should be private and hidden, but it is needed for tests
 mostly.

 > '''src/lib/datasrc/datasrc_messages.mes'''
 > How does DATASRC_SQLITE_CLOSE differ from DATASRC_SQLITE_CONNCLOSE (and
 DATASRC_SQLITE_OPEN differ from DATASRC_SQLITE_CONNOPEN)?

 One is for the older sqlite3 data source and one for this new one. The
 older should be removed once this one is finished.

 > '''src/lib/datasrc/sqlite3_connection.cc'''
 > '''src/lib/datasrc/sqlite3_connection.h'''
 > > I did copy paste a lot of code (and found a bug that the hasExactZone
 does not honor the class at all), and some of it is not needed yet. I put
 it into a comment for now so it doesn't have to be found in the original
 code every time. I'd like it to either get uncommented soon or deleted if
 it turns out it is not needed.
 > There appears to be a lot of common code between sqlite3_connection.cc
 and sqlite3_datasrc.cc.  Is the former a replacement for the latter?

 Yes, I hope to remove the sqlite3_datasrc.{h,cc} completely after the
 refactoring is done.

 > > I don't think the two-phase initialization of constructor and calling
 init is needed, so I put the parameters to the constructor directly. If
 the configuration is changed, a new connection can be created and the old
 one destroyed. I believe it is simpler this way, but I don't know what was
 the original idea here.
 > I would guess to avoid an exception being thrown from the constructor.

 What is wrong with that?

 > Regarding the configuration change, there is a comment in the
 !DatabaseClient header to the effect that objects returned hold references
 to the connection.  If there is such an outstanding object when a
 configuration change comes it, destroying the old connection and creating
 a new one could lead to problems.

 I believe the object should be short-lived, only for one query. At last
 the current usage indicates it. So, if it is reconfigured between queries,
 it shouldn't be problem. But currently, with the shared pointer, it
 shouldn't have the problem any more anyway.

 > > The constructor takes the config now. Maybe providing the needed data
 directly would be better, so it would be config-format independent and the
 thing creating it would handle configs?
 > I agree - it would be better to pass the name of the database file to
 the constructor and avoid the need for the class to know anything about
 the way BIND 10 stores its configuration.

 Updated.

 > > Do we want to support custom database schemas (so that user would
 provide the queries to DB in some kind of config) in future? I guess it's
 too early to add support for it now, but it looks useful if user has some
 kind of bigger database (but maybe more in case of mysql or other server-
 client database).
 > I don't think so.  If nothing else, if the user is using another
 database they will need to their own module for it containing the
 database-specific calls.  If they are using their own schema, that
 information would be there.

 Well, it could be avoided. If a user has the data in sqlite3 database
 file, but the columns are named differently, he could just provide the SQL
 statements to be used to get a zone ID or lookup an RRset.

 > __SQLite3Connection::getZone()__
 > A comment as to what is happening here would be useful.  Although the
 logic is obvious, it take some time to locate the definition of q_zone_ in
 the file to find out what SQL was being executed.

 Something like this?

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