BIND 10 #1061: introduce DatabaseClient (or DatabaseDataSourceClient)

[BIND 10 Development]

#1061: introduce DatabaseClient (or DatabaseDataSourceClient)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  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 stephen):

 * owner:  stephen => vorner


Comment:

 '''src/lib/datasrc/database.cc'''
 When will the "TODO Implement" items get done - have tickets been created
 for them?

 '''src/lib/datasrc/database.h'''

 __class DatabaseConnection__
 It is not usual to have a space between the "~" and the class name in the
 name of the destructor.

 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.

 __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

 Exception information should be described using the \exception tag.

 Regarding the comment about shared_ptr, the overhead of using a shared_ptr
 is probably small compared with the overhead of accessing the database, so
 it does seem as if a shared_ptr would be better.

 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?

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


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



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

 > 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.

 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.

 > 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.

 > 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.

 __class SQLite3Connection__
 Exception information should be described using the \exception tag.

 There is a space after the "~" in the name of the destructor.

 __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.

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