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