Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1183 closed task (complete)

data source refactor refactor

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20110830
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

"We need to go deeper!"

During the handling of the 106X tickets, a few changes were deferred, partly so that the branches would not diverge too much, and partly because some of the branches already contained some parts that other branches could use for this.

Most importantly, the calls to searchForRecords() and the related getNextRecord() should be done in a RAII style. #1067 added an iterator-style return value for getAllRecords(), and we should reuse that for searhcForRecords() (which i propose to rename to getRecords()). getNext() would then be a method of the object searchFor() returns.

The find() method(s) naturally need to be updated for this. A number of checks and catches can then be removed, as the RAII idiom would take care of the cleanup that is now done in a number of catches.

Also, since 1067 has been changed slightly to conform interface-wise to the changes in 1062, we can now make the size of the columns array in getNext() compiler-enforcable (by making the function parameter a reference to a fixed-size array).

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:2 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

  • added getRecords(), based on the old searchForRecords, but returns an IteratorContextPtr? similar to getAllRecords(). Since getRecords takes the name, the implementation actually skips copying the name column on calls to getNext()
  • tests updated to use this interface, and now useless tests removed
  • removed the old api calls (searchForRecords(), getNextRecord(), and resetSearch())
  • getNext() takes a reference-to-array now so we can do compile-time checks
  • remove Name argument to getAllRecords (was unused an unnecessary, it already takes the zone id)
  • also took the chance to fix the memleak in failed creation of sqlite3accessor, by making dbparameters_ a scoped_ptr

comment:3 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:4 in reply to: ↑ 2 ; follow-up: Changed 8 years ago 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]);
    

comment:5 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:6 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

First, overall I like the new version.

yay :)

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

thanks

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.

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?

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

oh yeah, changed.

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

Actually, come to think of it, I don't think we need the default implementation
either; if an implementor of a new database backend wants some code to run
before all functions are done, it can also just define the methods and raise
NotImplemented? there (or maybe even just DataSourceError?).

Made them pure virtuals. Perhaps the NopAccessor? class is now mostly useless,
though it could also serve as a nice code example of a featureless
implementation :)

  • 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
    
    

ok

  • 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.
        };
    

ack, done

  • getRecords: This doesn't seem to be correct:
  • Likewise, the corresponding description for getAllRecords() doesn't make much sense:

ack. Redid the docs, also added a note about how the caller should expect any
exception to be thrown. This is currently not actually reflected in our own code
that calls getRecords, but depending on the higher-level it should perhaps go
there.

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

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.

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

only one 'problem' with that is that is needs to store some dummy value in the
case of getAllRecords()

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

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

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

ack, moved

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

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.

changed it to use example.org.sqlite, which contains 11 records.

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

No, I removed all impossible tests there but forgot to check if the rest was
actually useful now :) removed.

  • 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]);
    

Actually, initially I had a comment somewhere that this was intentional, but now
that we don't use them in the actual implementation anymore we can change them
here as well. Done.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by jinmei

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

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by jelte

Replying to jinmei:

Replying to 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 ;)

comment:10 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

merged, closing ticket

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

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

Ah, sorry, I was not clear enough. I meant describing this behavior
in the base class documentation so that all derived class
implementation follows it. Suggested text is:

         * Once this function returns false, any subsequent call to it should
         * result in false.  The implementation of a derived class must ensure
         * it doesn't cause any disruption due to that such as a crash or
         * exception.

If this looks okay, I'll push it to the master (as this is a simple
documentation-only fix).

And, now I happen to make a followup comment on this function, here's
another thing:

         * \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").

On further thought, we may need this property if and when we implement
"ixfr-from-differences" (and I think we want to implement it), so,
although it may still be too strict to request it for all derived
classes, it would be better to suggest ensuring it when possible at
least for the "iteration" case.

comment:12 in reply to: ↑ 11 Changed 8 years ago by jelte

Replying to jinmei:

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

Ah, sorry, I was not clear enough. I meant describing this behavior
in the base class documentation so that all derived class
implementation follows it. Suggested text is:

         * Once this function returns false, any subsequent call to it should
         * result in false.  The implementation of a derived class must ensure
         * it doesn't cause any disruption due to that such as a crash or
         * exception.

If this looks okay, I'll push it to the master (as this is a simple
documentation-only fix).

sure, go ahead

And, now I happen to make a followup comment on this function, here's
another thing:

         * \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").

On further thought, we may need this property if and when we implement
"ixfr-from-differences" (and I think we want to implement it), so,
although it may still be too strict to request it for all derived
classes, it would be better to suggest ensuring it when possible at
least for the "iteration" case.

it did cross my mind, but i wasn't sure whether we'd use 'normal' full iterate for that, as i figured it might need more restrictions for such a usage, and if they are heavy, perhaps we'd need a different type of iterator for that as well (optional for datasources that wouldn't want to support it). But i haven't fully thought about how to do this yet

comment:13 Changed 8 years ago by stephen

  • Estimated Difficulty changed from 0 to 4
Note: See TracTickets for help on using tickets.