BIND 10 #1183: data source refactor refactor

[BIND 10 Development]

#1183: data source refactor refactor
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  major  |             Milestone:
                  Component:  data   |  Sprint-20110830
  source                             |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 First, overall I like the new version.

 Second, (as usual) I made some minor fixes/cleanups directly in the branch
 (pushed).

 '''database.h'''

 - This is not really about this branch, but is this note for getNext()
 true?
 {{{
          * \note The order of RRs is not strictly set, but the RRs for
 single
          * RRset must not be interleaved with any other RRs (eg. RRsets
 must be
          * "together").
 }}}
   At least the MockAccessor for the test doesn't seem to conform this
   requirement, but the test still passes.  And, in fact, we cannot
   always assume this especially if we allow admins to update the DB
   contents directly via DBMS.
   On looking into the implementation further, we don't seem to need
   this requirement: for the case of find(), addOrCreate() deals with
   interleaved data.  In case of full iteration, I suspect we shouldn't
   care: xfrout, the would-be primary user of this interface, will
   simply feed the result to the xfrout stream whether or not the data
   is interleaved.

 - Again, not really for this branch, but based on the "keep it dumb"
   principle, the 'name' argument for getRecords() should be const
   std::string&, not Name.

 - Once again not really for this branch, but as commented the
   static_cast<void> trick seems to be unnecessarily dirty.  I guess we
   can safely move the definition to .cc where we can omit the
   parameters, and if it's correct, I'd suggest that.  Further, if
   getRecords() doesn't even work this accessor is mostly useless.
   Personally I'd rather simply make it pure virtual (without
   definition) and force the implementor to support it (or otherwise to
   give up supporting this type of database).  I actually also suspect
   the same argument applies to getAllRecords(), but that may be a
   different topic.

 - I'd avoid hardcoding magic numbers like "all 5 elements" or "the
   fifth column" (it's susceptible to future changes - consider we
   deprecate SIGTYPE_COLUMN in a future version, for example).  Instead
   I'd
   say, e.g.:
 {{{
          * getAllRecords(), all COLUMN_COUNT elements of the array are
 overwritten.
          * For a 'name iterator', created with getRecords(), the
 NAME_COLUMN-th column
          * is untouched, since what would be added here is by
          * definition already known to the caller (it already passes it as

 }}}

 - On a related note: now that we pass the array in a securer way,
   maybe we should make the definition of the size more consistent with
   the array organization by defining it in the enum rather than via a
   separate const variable?  i.e., instead of
 {{{
 +    static const size_t COLUMN_COUNT = 5;
 }}}
   do this:
 {{{
     enum RecordColumns {
         TYPE_COLUMN = 0,    ///< The RRType of the record (A/NS/TXT etc.)
 ...
         NAME_COLUMN = 4,    ///< The domain name of this RR
         COLUMN_COUNT = 5    ///< Must be the largest value of above + 1.
     };
 }}}

 - getRecords: This doesn't seem to be correct:
 {{{
      * DatabaseAccessor's ZoneIterator. It can be created based on the
 name
      * or the ID (returned from getZone()), what is more comfortable for
 the
      * database implementation. Both are provided (and are guaranteed to
 match,
      * the DatabaseClient first looks up the zone ID and then calls this).
 }}}
   shouldn't this be "based on the name *and* the ID"?  And, in that
   sense, the rest of this paragraph doesn't make much sense to me
   either.

 - Likewise, the corresponding description for getAllRecords() doesn't
   make much sense:
 {{{
      * DatabaseAccessor's ZoneIterator. It can be created based on the
 name
      * or the ID (returned from getZone()), what is more comfortable for
 the
 }}}
   It doesn't take a "name" at all at the least.

 '''sqlite3_accessor.cc'''

 There doesn't seem to be anything obviously wrong, but I have some
 comments about the design and implementation of
 SQLite3Database::Context.

 - Should we allow creating multiple iterator context from a single
   DatabaseClient at once?  At least it won't happen when we use it
   from find().  For zone iteration we might want to allow that, but in
   that case I suspect we'd rather "clone" the database connection.  If
   we restrict the use of the iterator context to be "singleton" per
   client, we can use a prepared statement, which would be a bit
   faster, and it may be important in the context of find().

 - As pointed out above, I'd suggest that Context() take "name" as
   std::string.  Also, we might want to store the passed string inside
   the class. If the std::string implementation uses refcount +
   copy-on-write, the overhead should be marginal, and we can then also
   change SQLITE_TRANSIENT in bindName to SQLITE_STATIC.

 - I'd do sqlite3_finalize() once getNext() results in SQLITE_DONE.
   Then even if an application is lazy and doesn't release the context
   soon we can at least release the now-unnecessary lock on the DB.

 - The call to sqlite3_finalize() in bindName() doesn't seem to be
   necessary; if I miss something and it's really necessary, then I
   suspect we also need it in bindZoneId().

 - Since convertToPlainChar() is only used from the Context class, we
   could make it a private method of the class.  Then we don't have to
   pass dbparameters to the function. (btw, as far as I can see the
   passed dbparameters should never be NULL so the NULL check seems to
   be redundant).

 '''database_unittest.cc'''

 - this comment seems incomplete (missing closing parenthesis?)
 {{{
             // the error handling of find() (the other on is below in
             // if the name is "exceptiononsearch" it'll raise an exception
 here
 }}}
   (actually it doesn't parse well either)

 '''sqlite3_accessor_unittest.cc'''

 - Not really for this branch, but I think it's not really appropriate
   to use a zone containing only one RR(set) for the getAllRecords()
   test.

 - Don't we need iteratorColumnCount any more?  The fact that getNext()
   doesn't throw is already checked in the "iterator" test.

 - On a related note: I'd avoid using magic numbers in iterator tests:
 {{{
     const size_t size(5);
     std::string data[size];
 ...
     EXPECT_EQ("example2.com.", data[4]);
     EXPECT_EQ("SOA", data[0]);
     EXPECT_EQ("master.example2.com. admin.example2.com. "
               "1234 3600 1800 2419200 7200", data[3]);
     EXPECT_EQ("3600", data[1]);
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/1183#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development