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):

 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.

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

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

 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.

 > > - 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. [...]
 >
 > only one 'problem' with that is that is needs to store some dummy value
 in the
 > case of getAllRecords()

 Ah, right.  While it's not ideal to introduce the dummy data in terms
 of cleanness, the space/speed overhead wouldn't be an issue for
 getAllRecords(), which would be used performance (relatively)
 insensitive situations.  I'd leave the decision to you, and I noticed
 you adopted it, and the revised code looks good.

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

 > > - 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().
 >
 > not adding it at bindZoneId() was an oversight, and it may be
 interpreting the
 > documentation overly strict, but the way i read it every call to
 > prepare_statement() MUST be cleared with a finalize() at some point, and
 since
 > we call prepare() and bind() methods from the constructors, anything
 after
 > prepare() should do finalize() before it raises and exception.

 Ah, okay.

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

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