BIND 10 #1062: add a base for DatabaseZoneHandle::find()

[BIND 10 Development]

#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:11 jelte]:

 > > Hmm, it doesn't seem to be changed...
 >
 > it appears I lost one of my commits :/ anyway. Unconsted again.

 Maybe we should explain the intent.  Proposed text (which should go to
 the ZoneFinder::find() documentation):

 {{{
     /// \note Maybe counter intuitively, this method is not a const member
     /// function.  This is intentional; some of the underlying
 implementations
     /// are expected to use a database backend, and would internally
 contain
     /// some abstraction of "database connection".  In the most strict
 sense
     /// any (even read only) operation might change the internal state of
     /// such a connection, and in that sense the operation cannot be
 considered
     /// "const".  In order to avoid giving a false sense of safety to the
     /// caller, we indicate a call to this method may have a surprising
     /// side effect.  That said, this view may be too strict and it may
     /// make sense to say the internal database connection doesn't affect
     /// external behavior in terms of the interface of this method.  As
     /// we gain more experiences with various kinds of backends we may
     /// revisit the constness.
 }}}

 > > Looks good, but I'd also include the class name and some identifier of
 > > the underlying database (maybe we need a method in
 > > DatabaseConnection?) [...]
 >
 > I'm afraid putting in the full path may be a bit long, so I added yet
 > another utility method to util/filename.h; getNameAndExtension, [...]

 Okay, and I have some suggested editorial/documentation updates and
 committed them to the branch.

 > > > > - in database.cc, I guess the exception/error handling needs to be
 > > > >   revised. [...]

 > > The revised code seems to do the right thing, but I'm personally
 > > concerned about the heavily nested try-catch logic:
 >
 > Yes, we should do RAII, and we should probably do it in a form where
 > searchForRecords returns some object you can call getNext() on
 (apparently
 > michal is creating something similar for #1067, and the base class seems
 the
 > same as what i need here).
 >
 > I am passing this back to you now though, though so you can look at
 these other
 > changes

 Since other things depend on this ticket/branch, I'm inclined to merge
 this at the moment and create a separate ticket for further
 consideration.  Even though the result of it may involve interface
 change and thus affect subsequent tasks, I guess it's more efficient
 way of development.  But if you want to fix that point within this
 ticket, that's fine to me, too.

 > > 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.
 >
 > ack

 Hmm, it's now RECORDCOLUMNCOUNT...isn't it a bit unreadable?  If the
 total length due to underscores is an issue, maybe we use RECORD_COUNT
 or COLUMN_COUNT?  (I guess the semantics should probably be
 sufficiently clear where it's used).

 > > BTW maybe we want to log the event when we perform setTTL()?
 >
 > good idea (long discussion on jabber room about level. no real
 consensus, making
 > it INFO (but I can easily see it being DBG_TRACE_BASIC as well)
 >
 > btw, i also changed the if; if we have such a message we should also
 print it when the first TTL happens to be the lowest one

 Okay.  As for the log message, we should probably add DBName too.

 > > Also note you may want to use sqlite3_errmsg() instead of playing with
 > > the result code directly (see above).
 >
 > yes, though here it is slightly trickier, [...]
 > and do resetSearch() there, I just removed the resetSearch() here.
 > (the other option would be to copy the errmsg() result first)

 I think it's okay.  The caller can encounter an exception with the
 underlying lookup/transaction still active (e.g. when the stored data
 is bogus wrt the DNS protocol) in which case the app is responsible
 for resetting the context anyway.

 > > > >   - if not very hard, I'd check RDATA(s), [...]

 > oh, oops, somehow read over this one. Added it by passing a vector of
 > expected_rdatas and expected_sig_rdatas; the expected_counts that were
 there are
 > now unnecessary (same as size() on those vectors), so i removed those as
 > arguments. (i did not want to do something clever with the data as it is
 already
 > in the database; as that could potentially cause internal problems we
 miss, like
 > data not being checked at all)

 Okay.  I have one comment regarding the new tests: in doFindTest(),
 you can omit checking these because it's done in rrsetCheck:

 {{{
         EXPECT_EQ(expected_rdatas.size(), result.rrset->getRdataCount());
         EXPECT_EQ(expected_ttl, result.rrset->getTTL());
         EXPECT_EQ(expected_type, result.rrset->getType());
 }}}

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