BIND 10 #374: Writable datasources: Basic support for writables in C++ datasrc library
BIND 10 Development
do-not-reply at isc.org
Thu Nov 11 13:19:41 UTC 2010
#374: Writable datasources: Basic support for writables in C++ datasrc library
------------------------------+---------------------------------------------
Reporter: jelte | Owner: UnAssigned
Type: enhancement | Status: reviewing
Priority: major | Milestone:
Component: data source | Resolution:
Keywords: | Sensitive: 0
Estimatedhours: 0.0 | Hours: 0
Billable: 1 | Totalhours: 0
Internal: 0 |
------------------------------+---------------------------------------------
Comment(by jinmei):
continuing review...still ongoing.
'''data_source.h'''
- (forgot to point this out) in the declaration of
DataSrc::startTransaction(), if you specify 'const' for
'create_zone', you'll have to be consistent about that in its
definition (and possibly the declaration and definition of
derived versions) due to a deviant (or buggy) behavior of
sunstudio.
'''sqlite3_datasrc.cc'''
- boost/foreach.hpp doesn't seem to have to be included.
- in ~Sqlite3Initializer(), shouldn't we handle w_add_all_, etc, too?
and, this seems to indicate that it's time to introduce some
refactoring to make the code robust.
- Sqlite3DataSrc::open(): I'd make sq_result 'const int'. I'd fold
the isc_throw() line for readability.
- Sqlite3DataSrc::close(): w_del_all_ and w_del_zone_ are not
cleared. perhaps another indication of the need for refactoring
(see above).
- Sqlite3DataSrc::startTransaction()
- I'd throw an exception if state != INIT as it should be a program
error at the caller side. same for commit/rollback.
- we could store the result of transaction.getZoneName().toText()
so that we can reuse it in the new zone case. just a comment.
- shouldn't we expect zone_id == 0 as a valid case?
{{{
if (zone_id <= 0) {
}}}
- why do we assume the RRClass is IN?
{{{
DataSrc::WriteResult add_zone_res =
addZone(transaction.getZoneName(),
isc::dns::RRClass::IN());
}}}
- I'd make sure zone_id is valid after creation:
{{{
zone_id =
hasExactZone(transaction.getZoneName().toText().c_str());
assert(zone_id >= 0);
}}}
- how could the rollback fail in this context? depending on the
reason, it may be better to handle it differently than returning
the generic error code, e.g. throw an exception.
- I'd avoid hardcoding keyword "zone_id" for trans_data. same for
other part of the code that uses this keyword.
- Sqlite3DataSrc::commitTransaction()
- I'd declare 'result' as 'const int' or even omit the use of a
temporary variable. same for rollback.
- Sqlite3DataSrc::addZone()
- in the first call to sqlite3_bind_text(), it seems that the last
parameter can be SQLITE_STATIC. Also, if we use TRANSIENT, we
can avoid having a temporary variable (s_name), although I
wouldn't be so nervous about a "redundant" variable if it's a
const.
Furthermore, the mixed usage of STATIC and TRANSIENT may confuse
code readers (and might trigger an accidental regression in future
by a careless developer). Ideally we should keep the consistent
style, or at least leave a comment about our policy of when to use
STATIC/TRANSIENT. (one possible approach is to use TRANSIENT
consistently in the "write" operations because its performance
impact should be relatively less important).
This comment applies to some of the other cases in other methods.
- what should we do if the zone for the <name, rrclass> already exists?
- I'd avoid using a temporary mutable variable 'rc' if we don't use it
except for '!= SQLITE_OK'. same for 'rc' in other methods.
- I'd declare 'result' as const int.
- Sqlite3DataSrc::addRRset()
- I'd throw an exception if getRdataCount == 0 as it should be a program
error at the caller side.
- zone_id could (should) be 'const int'
- in this context we cannot naively assume getData() or
get("zone_id") returns a valid value. (see also the higher level
comment about the design)
- Sqlite3DataSrc::delRR(without rdata)
- looks like the third call to sqlite3_bind_text can fit in a single
line:
{{{
rc = sqlite3_bind_text(query, 3, "%", -1, SQLITE_STATIC);
}}}
- is it okay to use sqlite3_total_changes() for this purpose?
according to the sqlite3 manual, it's the number of changes "since
the database connection was opened". btw, if my concern is real,
that would suggest unit tests may not be sufficient.
- I'd declare 'deleted_rows' as 'const int'.
- not specific to this method, but as I've seen so many repetitive
patterns around sqlite3_bind_text, I wonder we might unify this
pattern to avoid repeating the duplicate logic. e.g, we might
define something like
{{{
void bindName(sqlite3_stmt* query, const string& name_str, const int
column) {
if (sqlite3_bind_text(query, column, name_str.c_str(), -1,
SQLITE_STATIC) != SQLITE_OK) {
isc_throw(...);
}
}
void bindName(sqlite3_stmt* query, const char* const name_cstr, const
int column) {
if (sqlite3_bind_text(query, column, name_cstr, -1,
SQLITE_TRANSIENT) != SQLITE_OK) {
isc_throw(...);
}
}
}}}
then we can simplify each method like as follows:
{{{
...
bindZone(query, 1, zone_id)
bindName(query, 2, name.toText().c_str());
bindRRType(query, 3, RRType::ANY() ? "%" : rrtype.toText().c_str());
...
}}}
just a thought.
- Sqlite3DataSrc::delRR(with rdata)
- most part of the code is a duplicate of the non rdata version.
smells like the need for refactoring.
- Sqlite3DataSrc::delAll()
- at the end of the method, for such a simple if-else pattern I'd
use ?: to keep the size of the method shorter:
{{{
return (result == SQLITE_DONE ? W_SUCCESS : W_DB_ERROR);
}}}
but I understand this is largely a matter of preference. it's just
a comment. oh, btw, in the scope of Sqlite3DataSrc you can ommit
"DataSrc::".
- Sqlite3DataSrc::replaceZone()
- I'd thrown an exception if state != RUNNING.
- getData() and get("zone_id") may return an invalid value.
- is there a valid scenario that we see NULL for nextRRset? if not,
I'd rather throw an exception...hmm, DataSrc::doIXFR() has that
usage, but in data_source.h it's not clear what we'd expect when
nextRRset is NULL.
- I'd declare next_rrset as ConstRRsetPtr.
- Sqlite3DataSrc::delZone()
- I'd thrown an exception if state != RUNNING.
- getData() and get("zone_id") may return an invalid value.
- hmm, I now realize this method (and most if not all of the
writable variants of the methods) itself doesn't provide strong
exception/failure guarantee, i.e, when something bad happens the DB
may stay in a strange state. it should be okay because it's
assumed to be used with a transaction context, but I think it's
better to note that fact explicitly. (see also below for the
relevant comment about sqlite3_datasrc.h)
- Sqlite3DataSrc::delRRset()
- same usual comments as delZone()
'''sqlite3_datasrc.h'''
- we should be able to omit including most of the added .h's. as far
as I can see the only necessary header file is rrset.h for RRsetPtr.
for others more lightweight forward declaration should suffice.
- The public methods do not simply follow the interface defined in
the base class, but have some sqlite3 specific behavior (especially
about when to throw exceptions). So I think we should provide
documentation for these methods focusing on the specific behavior
while referring to the base class.
- indentation isn't well aligned for delRR() (both of the two variants).
(to be continued)
--
Ticket URL: <http://bind10.isc.org/ticket/374#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list