BIND 10 #2541: add addZone() interface for data source
BIND 10 Development
do-not-reply at isc.org
Tue Dec 11 19:27:10 UTC 2012
#2541: add addZone() interface for data source
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: data source | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20121218
Sub-Project: DNS | Resolution:
Estimated Difficulty: 0 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| loadzone-ng
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Comment (by jinmei):
'''client.h'''
- as for providing the default definition in the base class (and still
not making it pure virtual): In general, I've been trying to avoid
it so that these default versions are not used accidentally (at the
cost of requesting defining something in each derived class). I'd
also generally avoid relying on the "not implemented" default,
because it's quite likely to suggest that the abstraction of the
base class isn't really good. But for this particular case we're
doing some kind of hack, which is a bit better than the famous
"short term workaround", so, assuming we shouldn't be afraid of
totally revisiting it, I'm okay with breaking these two principles.
But I'd suggest adding some considerations as a comment related to
this. (See also the next bullet for providing the default
definition).
- parameter `name` doesn't appear in the declaration. To avoid that
while also avoiding to have 'unused parameter' warning, we'd need to
move the definition to a .cc file.
- "fully qualified" sounds redundant to me, because a `Name` object
always represents an absolute name. (but this is a minor point)
'''database.h'''
- addZone() declaration: shouldn't this `may` be `will` or `must` or
something?
{{{#!cpp
/// Callers must also start a transaction before calling this method,
/// implementations may throw DataSourceError if this has not been
done.
}}}
- createZone() declaration: I'd skip describing param and return if
there's nothing special to the database version (to avoid repeating
the same thing).
'''database.cc'''
- `TransactionHolder` destructor: rollback() can throw, so we need some
consideration for that case. See also `~DatabaseUpdater()`.
'''sqlite3_accessor.h'''
- Per style guide, we'd use C++-style comments for doxygen
{{{#!cpp
/**
* \brief Add a zone
*
}}}
Maybe you were trying to be consistent with the style for the
previous method, but consistency is already broken within the file,
and since this branch isn't big, I'd suggest providing consistency
throughout the file (by changing others too).
- For `SQLite3` specific text, "empty" looks awkward:
{{{
* This implements the addZone from DatabaseAccessor and adds an
(empty)
* zone into the zones table. If the zone exists already, it is still
}}}
because this concept can't be represented in the `zones` table. I'd
simply say "adds a zone" here.
'''sqlite3_accessor.cc'''
- If we require a transaction in addZone(), I think something like
`InvalidOperation` is a more appropriate exception for the
violation (although I've noticed there are other similar cases in
this file where the same argument could apply).
- I believe these could be SQLITE_STATIC (which is a bit more efficient)
in this case:
{{{#!cpp
proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
}}}
in fact, I think TRANSIENT is a better choice in this case because
it's safer and performance isn't important here. But it's probably
a good idea to explicitly note that in a comment to prevent someone
from trying to unnecessarily "optimize" it later.
'''sqlite3_accessor_unittest.cc'''
- Overall: I guess many if not all of ASSERT_xx can be EXPECT_xxx.
- I'd check the existence of the same name of zone but of a different
RR class doesn't cause confusion
- If possible I'd like to test the case where either/both of
add/getZone fails (maybe possible with another write).
'''database_unittest.cc'''
- I'd like to test the case where rollback happens.
--
Ticket URL: <http://bind10.isc.org/ticket/2541#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list