Opened 9 years ago

Closed 9 years ago

#1061 closed task (complete)

introduce DatabaseClient (or DatabaseDataSourceClient)

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20110816
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.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

In this task, we add a minimum level of additional support for
SQLite3, implementing the following:

  • DatabaseConnection? (abstract) class. At the moment we only need the constructor and the getZone() method.
  • SQLite3Connection class. Implement the constructor, which opens the underlying SQLite3 DB, and getZone() method.
  • Implement findZone() method. At this point the returned ZoneHandle? (DatabaseZoneHandle?) can be empty.

For SQLite3 specific code, use the exiting sqlite3_datasrc
implementation when possible.

This task depends on #1060.

Subtickets

Change History (17)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5

comment:2 Changed 9 years ago by stephen

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

comment:3 Changed 9 years ago by vorner

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

comment:4 Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to assigned

Hello

It should be ready for review. The generic part should be OK, but I'm not sure about few details with the SQLite3Connection:

  • 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.
  • 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.
  • 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?
  • 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).

comment:5 Changed 9 years ago by vorner

  • Status changed from assigned to reviewing

comment:6 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to vorner

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.

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

  • Owner changed from vorner to stephen

Hello

Replying to 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?

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

Replying to vorner:

Not intending to steal the review, but I thought I might be able to
clarify some points in the discussion, so...

Exception information should be described using the \exception tag.

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

That would be helpful if and when we introduce the Python script that
converts C++ doxygen to a template Python docstring (for the
corresponding C++ binding). (There's a ticket for this: #933).

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?

I don't remember why the original code adopted the two-phase
initialization (constructor and init()), but at least for the design
concept of DataSourceClient?, I suspect we don't need the phased
initialization. If there's a need for reinitializing it, we'd
probably reconstruct a new client.

And, as far as I can see, there should be no problem in having the
possibility of throwing an exception from 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.

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.

Is it about DataSourceClient?? If so, no, the intent was that an
application will keep a client as long as possible (for performance
reasons). ZoneFinder? objects are expected to be short lived.

comment:10 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to vorner

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

I think a different name would be better. A database connection has a special meaning; using the term for a class in BIND 10 runs the risk of confusion with the underlying database connections.

(regarding comment in header for FindZone)
Should I reword it somehow?

I think so. I'm not clear what the restriction on the returned object is or why it is there.

class DatabaseClient::Finder
getOrigin(), getClass() and find() do not have header comments.

Do they need one?

In that case, a comment that these are methods defined in a base class would be OK.

File changes are OK.

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

  • Owner changed from vorner to UnAssigned

Replying to stephen:

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

I think a different name would be better. A database connection has a special meaning; using the term for a class in BIND 10 runs the risk of confusion with the underlying database connections.

OK, I renamed it to DatabaseAbstraction?, it is hopefully better. But the two current follow-up tickets will need updates :-(.

Assigning to unassigned, as Stephen is away.

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

Replying to vorner:

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

I think a different name would be better. A database connection has a special meaning; using the term for a class in BIND 10 runs the risk of confusion with the underlying database connections.

OK, I renamed it to DatabaseAbstraction?, it is hopefully better. But the two current follow-up tickets will need updates :-(.

Assigning to unassigned, as Stephen is away.

Okay, so I'll steal it:-). I'm focusing on the class naming issue.

I think I understand what Stephen wanted to point out, and I see some
point in it, but I'm not sure if the name was so confusing. The
intent of the getZone() method is to "get zone via the connection to
the database", and it doesn't seem to look so awkward.

I don't mind changing the class name if we come up with something
clearly better, but I have some concern with "DatabaseAbstraction?".

First, it sounds like a database system itself while it's actually
indeed a "connection", so for example it would look a bit awkward when
we have multiple instances of (a derived class of) DatabaseAbstraction?
connecting to the same database. "DataSourceClient?" was named a
"client" to address the same concern, and the same sense applied to
"DatabaseConnection?".

Second, "abstraction" can be ambiguous. Since this is an abstract
base class, "abstraction" may simply sound indicating that fact (but
that would make it look a bit inconsistent because other base classes
that are used with the "DatabaseSomething?" class are not named with
"abstract").

After thinking about it more, I came up with another possible name
"DatabaseAccessor?". It may share the same concern as that for the
original "DatabaseConnection?", but would this one perhaps sound
better (while addressing my own concerns)?

Or is there anything that can better address both concerns?

comment:13 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to vorner

comment:14 in reply to: ↑ 12 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

After thinking about it more, I came up with another possible name
"DatabaseAccessor?". It may share the same concern as that for the
original "DatabaseConnection?", but would this one perhaps sound
better (while addressing my own concerns)?

Or is there anything that can better address both concerns?

Well, I used Abstraction, because I couldn't come with anything better, Accessor looks slightly better than that. I believe it addresses Stephens concern as well, as database accessor isn't something commonly used to name something. So I used your idea.

Thank you

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

Replying to vorner:

After thinking about it more, I came up with another possible name
"DatabaseAccessor?". It may share the same concern as that for the
original "DatabaseConnection?", but would this one perhaps sound
better (while addressing my own concerns)?

Or is there anything that can better address both concerns?

Well, I used Abstraction, because I couldn't come with anything better, Accessor looks slightly better than that. I believe it addresses Stephens concern as well, as database accessor isn't something commonly used to name something. So I used your idea.

Okay, then the branch is basically ready for merge, but I noticed some
of the variables and test case names still contain "conn" etc. They
should better be renamed accordingly. I don't think we need another
cycle of review just for that. Once the changed code passes compile
it should be able to be merged.

comment:16 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:17 Changed 9 years ago by vorner

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

I tried to find what I could and merged. Thanks.

Note: See TracTickets for help on using tickets.