BIND 10 #1183: data source refactor refactor

[BIND 10 Development]

#1183: data source refactor refactor
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jelte
                       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 jelte):

 Replying to [comment:7 jinmei]:
 > Replying to [comment:6 jelte]:
 >
 > The revised version basically looks okay.  The only possibly blocking
 > issue is what happens if getNext() is called after it returns false
 > (see below).  But I'll be off for a few days, I'm okay with merging
 > the current version and deferring this particular topic, or with
 > fixing the issue at your discretion and merging the result.
 >

 after the addition of the private finalize(), which sets stmt_ to NULL,
 having a
 check for this was trivial, so I'll assume this was OK and will merge
 shortly

 > > > - 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").
 > > > }}}
 > [...]
 > > Good points. For either use of it we have it does indeed not really
 matter. It
 > > should however not matter whether or not the DBMS is modified
 externally; it is
 > > a property that the implementation should (or should not have to)
 provide. So
 > > since find() actually doesn't care about this property, the only real
 reason to
 > > leave it in would be so that we can ensure that the 'full iterator'
 returned by
 > > getAllRecords() always returns complete RRsets. If we only intend to
 use this
 > > for xfrout (or similar functionality) we don't need it. I have no
 strong
 > > opinion, should I remove the requirement?
 >
 > To me it seems to be unnecessarily restrictive.  On looking into the
 > implementation more closely, I found that this requirement is actually
 > met in the sqlite3 accessor by specifying "ORDER BY name, rdtype", but
 > depending on the characteristics of the underlying database system it
 > may not always be easy/feasible/efficient.  In any case, this is not
 > directly related to this branch, so my suggestion is to discuss it
 > with Michal separately when he's back (maybe we should create a ticket
 > for the record).  My personal opinion is that we should remove the
 > restriction until/unless we know something wrong happens without it.
 >

 ok

 > > > '''sqlite3_accessor.cc'''
 >
 > > > - Should we allow creating multiple iterator context from a single
 > > >   DatabaseClient at once?  [...]
 > > >
 > >
 > > I believe Michal had a use-case for that (though I'm not sure what it
 was, and
 > > whether that use-case was just the ability to do so itself). I must
 say that,
 > > apart from the obvious performance penalty, I do like the cleanliness
 of having
 > > them prepared per use, instead of per connection (and having all init
 and
 > > cleanup on the same level, as it were). Apart from that I have no
 strong opinion
 > > on it.
 > >
 > > For now I have left this as it is, but I'm open for discussion.
 >
 > Okay.  I don't request it to be resolved within ticket.  As for
 > performance penalty, it may not matter in the end because it's quite
 > likely that we'll need to heavily rely on hot spot caching for
 > relatively performance sensitive environments.  For the "all record"
 > case, I guess we'll need "cloning", which I'll introduce for
 > writability anyway, and we can then think about reusing it for that
 > case.
 >

 ah cool :)

 > At the moment my suggestion is to open a separate ticket about the
 > performance consideration so that we can revisit the issue when we
 > work on performance improvement.
 >

 done (#1188)


 > > > - 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.
 > >
 > > done, moved it to a private method btw, which also sets the pointer to
 NULL.
 >
 > Looks good, and I now wonder whether it's okay to call getNext() after
 > it returns false.  We could either make it no-op or throw an
 > exception, but if it currently causes any disruption such as segfault
 > we should prevent that.  Also, it should be included in the abstract
 > level interface.
 >

 returning false again seems most natural (even though it is really a
 programming
 error, false for 'yeah yeah you are still done' seems more logical to me).
 Putting it in the abstract might be a tad difficult if we don't have a
 virtual
 implementation :)

 > > > '''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)
 > > >
 > >
 > > updated the text, but actually, i am wondering if we should still have
 them, as
 > > these exceptions are no longer explicitely caught, making the test
 effectively
 > > only test whether these exceptions are not caught...
 >
 > "exceptions are throws" should be "exceptions are thrown".  I'd leave
 > it to you whether to clean up the not-so-useful test cases.

 Updated, but left them in. Should we decide we do want to catch them at
 this
 level, the failing unit tests will remind us to specifically test for it
 ;)

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