BIND 10 #1062: add a base for DatabaseZoneHandle::find()
BIND 10 Development
do-not-reply at isc.org
Thu Aug 11 09:14:01 UTC 2011
#1062: add a base for DatabaseZoneHandle::find()
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
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 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:8 jelte]:
A few preliminary notes:
- (as always) I made some minor editorial changes
- please note that in #1061 it's likely we'll change "connection" to
"accessor". some of internal variable names, test cases, etc. may
have to be adjusted as a result.
- I know comments below are not short, but most of them are quite
minor or trivial to address. The (almost) only real point to
discuss is the use of try-catch in find().
> > '''General Discussions'''
> >
> > - I agree with changing getNextRecord() to a non const member function
> > for the reason you explained. But this would also mean find()
> > shouldn't be a cost member function of DatabaseClient::Finder (and
> > therefore, of ZoneFinder). [...]
>
> ok, unconsted it
Hmm, it doesn't seem to be changed...
> > - Don't we need to use loggers in database and sqlite3_connection?
> >
>
> hmm, yes, completely forgot about those :/
>
> I added some in find(), it'll log either a successful return, or one of
the
> errors as caught by the big try/catch introduced in this revision.
Looks good, but I'd also include the class name and some identifier of
the underlying database (maybe we need a method in
DatabaseConnection?) for all these log messages, e.g.:
{{{
logger.debug(DBG_TRACE_DETAILED,
DATASRC_DATABASE_FIND_RECORDS).arg(name).arg(getClass()).arg(type).arg(connection_->getDBName());
}}}
(I assume a new method "getDBName()", which would result in something
like "sqlite3 (+ perhaps filename)")
> > - in database.cc, I guess the exception/error handling needs to be
> > revised. [...]
>
> I've implemented this approach for now, doing it with RAII is probably
better,
> but may mean some other interface changes as well, and probably an
entire
> different approach to the way we use the sqlite stmt variables right now
> (although i guess some of the potential problems i see would also be
present
> now)
The revised code seems to do the right thing, but I'm personally
concerned about the heavily nested try-catch logic:
- the outer most one in find()
- the one in the while loop
- the one in addOrCreate(), which is called in the second try block
In general, if we catch specific exceptions at each logical level like
this, the resulting code will be quite similar to the plain old
C-style error handling, where the normal code logic and error cases
are quite mixed and the code is not very readable.
In fact, due to the many catch-es, the find() method is already pretty
big, and I suspect it will be even more complicated as we implement
more search logic (longest match, wildcard, delegation, etc), although
at that point we'll need to refactor the method with some smaller
helper private methods anyway.
I guess you caught exceptions in every level so that you can provide
detailed information about what's wrong in the database when some
bogus data is returned from the backend as you commented:
> Actually, while the higher-level catch should at clean this up
correctly,
> i also added a specific exception here (next to InvalidRRType and
InvalidRRTTL),
> where we still have access to the source data, for a better exception
message.
I see the point of the motivation, and I do not necessarily say it's a
bad idea, but personally the current balance of complexity and the
benefit of providing detailed error info is not good.
Now, if my argument is convincing and you agree with simplifying the
try-catch block, that's of course fine to me.
If not, which is also understandable,
- if you can come up with a middle ground solution, that would be nice.
- otherwise, let's leave it for now (maybe we can revisit it as we add
more logic to find()), but please leave some comments about the pros
and cons of this method and the rationale of the current decision.
> I did add some tests that make sure resetSearch() (the cleanup method)
is
> called, both on completion of a call to find() and on any exception that
is
> raised.
Regarding tests for resetSearch(): why are some of the tests commented
out?
{{{
// Trigger the hardcoded exceptions and see if find() has cleaned up
/*
EXPECT_THROW(finder->find(isc::dns::Name("dsexception.in.search."),
isc::dns::RRType::A(),
NULL,
ZoneFinder::FIND_DEFAULT),
DataSourceError);
...
}}}
> > '''database.h/cc'''
> >
> > - maybe we want to use a plain old array instead of vector for
> > columns, if we are sure it has a fixed size? [...]
>
> Hmm, yes, a direct array is probably better, though also slightly
> more dangerous. Therefore I also added a second argument, the
> vector size. Doesn't provide full protection against passing in a
> smaller array, but at least it's something; the dabaseimplementation
> should check it.
If you like the efficiency of the array but want to make it safe, an
alternative would be to use boost::array (at the cost of the
additional dependency) and use at() instead of operator[] inside the
callee. I don't have a strong opinion, and leave it to you.
> > - (general) I think it's better to define an enum or something to
> > represent the semantics of the vector used in getNextRecord(),
> > rather than hardcoding the magic numbers:
> > {{{
> > if (columns.size() != 4) {
> > ...
> > const isc::dns::RRType cur_type(columns[0]);
> > const isc::dns::RRTTL cur_ttl(columns[1]);
> > }}}
>
> ack, done
Looks good, but I guess RecordColumnCount should also be all upper
cased (with _, i.e. RECORD_COLUMN_COUNT) as it's a constant according
to the coding guideline.
Also, this comment seems incomplete:
{{{
TTL_COLUMN = 1, ///< The TTL of the record (a
}}}
> > - addOrCreate(): it's not clear how it's possible that type !=
rrset->getType():
[...]
> > (BTW, lcov indicates this case isn't covered by tests)
>
> yes well with the current code it shouldn't be possible, and since
> addOrCreate is in anonymous namespace we can't directly call it (or
> did we have a clean way to do that?), so it's not covered by a
> test...
>
> made it an assert.
With the revised (with assert), I think code missing the test is okay.
One thing I didn't notice in the previous code: the assert() should
probably be better before the TTL check:
{{{
// This is a check to make sure find() is not messing things up
assert(type == rrset->getType());
if (ttl < rrset->getTTL()) {
rrset->setTTL(ttl);
}
}}}
BTW maybe we want to log the event when we perform setTTL()?
> > '''sqlite3_connection.cc'''
> >
> > - In searchForRecords(), sqlite3_bind_text() can fail (and
> > sqlite3_bind_int() may also fail in theory?), and we should catch
> > the case even though it would be a rare event.
>
> ack, done
Looks good, but in #1068 I noticed we might rather use
sqlite3_errmsg() rather than including the raw result code. That is,
instead of:
{{{
int result;
result = sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id);
if (result != SQLITE_OK) {
isc_throw(DataSourceError,
"Error in sqlite3_bind_int() for zone_id " <<
zone_id << ", sqlite3 result code: " << result);
}
}}}
we do:
{{{
if (sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id) != SQLITE_OK)
{
isc_throw(DataSourceError,
"Error in sqlite3_bind_int() for zone_id " <<
zone_id << ": " << sqlite3_errmsg(dbparameters_->db_));
}
}}}
This way, we can eliminate the temporary variable, can make methods
shorter, and can provide more human readable what() message.
> > - convertToPlainChar(): (I'm asking partly because I forgot:-) in
> > which case can ucp be NULL? Also, the main purpose of this helper
> > function is to work around the C++ strictness on the difference
> > between 'unsigned char*' and 'char *' (sqlite3 uses the former
> > throughout its API, while std::string accepts the latter). And,
> > from a point of type safe purist this conversion isn't correct
> > (because in theory it's not guaranteed that these two pointer types
> > are convertible). I'd note these points as comments.
>
> IIRC the return value from sqlite_column_text() can be NULL if the
database
> field itself was NULL or if it has run out of memory. [...]
Ah, okay, I experimentally commented out the NULL check and saw the
tests triggered segfault:-) Hmm, so, technically, I think we
should separate these two cases: if ucp == NULL and sqlite3_errcode()
is NOMEM we should probably throw an exception rather than silently
convert it to ""...
> I've added some comments. BTW, I can also get around the conversion by
using
> string::assign(), which makes the compiler do its magic on its own. But
explicit
> casts are probably better :)
Oh, right. When you use assign() you can change the pointer
conversion to plain char conversion:
{{{
for (i = 0; i < num_of_chars; ++i) {
string.internal_char_buf[i] = unsigned_char_buf[i];
}
}}}
(but I think we'll keep casting in this case anyway)
> > - getNextRecord(): although it should be rare and might be purely in
> > theory, this method can internally throw an exception (bad_alloc).
> > we should probably catch that within this method and convert it to
> > DataSourceError so that the application can handle that situation
> > more gracefully (e.g., exit after closing the DB connection).
>
> done
bad_alloc can happen only here:
{{{
columns[column] = convertToPlainChar(sqlite3_column_text(
current_stmt,
column));
}}}
so we could narrow the try-catch scope. But you might rather want to
avoid missing a non obvious case, so I'd leave it to you.
BTW, I now notice one another point in this method: we should probably
do validate column_count before sqlite3_step.
Also note you may want to use sqlite3_errmsg() instead of playing with
the result code directly (see above).
> > - not a big deal especially in tests, but if I understand it correctly
> > "conn" can be scoped_ptr instead of shared_ptr:
>
> ack, changed. Actually, for countRecords, see below.
Okay, and I suspect you can remove this:
{{{
#include <boost/shared_ptr.hpp>
}}}
> > - if possible, I'd like to test the case where getNextRecord() results
> > in DataSourceError().
[...]
> I do not know of any dependable way to make sqlite3_step return an
error, though...
With luck I guess we could make it fail with 'busy', but I admit we
probably cannot do it reliably. So, it's okay.
> > - if not very hard, I'd check RDATA(s), not only the number of
> > RDATAs and counts. maybe we could use rrset[s]Check defined in
> > lib/testutils/dnsmessage_test.h.
This doesn't seem to be addressed. Did you think it too hard?
--
Ticket URL: <http://bind10.isc.org/ticket/1062#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list