BIND 10 #1607: define ZoneFinder::findAdditional() and implement its default version

[BIND 10 Development]

#1607: define ZoneFinder::findAdditional() and implement its default version
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120306
                  Component:         |            Resolution:
  b10-auth                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 vorner]:

 Thanks for the review.

 > I'm thinking, if we're returning such a complicated result already, do
 we need a special interface for findAll? Wouldn't it be better to just
 have the context be able to fill all the results into a target passed to
 it, no matter if there's one or many? The context already has a copy of
 the vector for the findAll case anyway and it could simplify processing
 both inside the context and outside.

 Hmm, good point.  I didn't think about it but that seems to make some
 sense.  I personally still think it makes sense to define findAll()
 separately from find() rather than relying on the implicit meaning of
 qtype=ANY, but at least we can unify the interface.

 Would you like to introduce the change now, or should it go to a
 separate ticket?

 > Another thing, maybe we want to have a abstract base class that contains
 no data members like the vector for the all thing. I could imagine the in-
 memory remembering the node only and iterating that one when needed,
 instead of copying it out to a vector to iterate later.

 Right.  I was aware of that, and in fact I liked the clearer
 separation of abstract interface from stuff that may not be used by
 all derived classes.  I still pushed vector, etc, to the base class
 because if a derived class wants partial customization (e.g., use
 customized version for getNegativeProof() if it's ever introduced but
 still use the default version of getAdditional()), it'll still needs
 to get access to things for the default version.  I also expect if we
 extend the context it may need to remember other information such as
 the original qname (in case wildcard expansion happens), and so the
 base class would retain a pointer blob, such as:
 {{{#!c++
 class Context {
 ...
    BaseContextData* base_data;
 };
 }}}
 and allow a fully-customized implementation to keep it NULL.

 Alternatively, we could define a protected method such as
 getAllRRsets(), getFindOptions(), etc, and the default implementation
 of getAdditional() would use getAllRRsets() to get access to the
 RRsets that were possibly found by findAll().  Then we can move the
 member variables to a separate derived class:

 {{{#!c++
 class Context {
 ...
 protected:
     virtual vector<ConstRRset>* getAllRRsets() const = 0;
     virtual getAdditional(...) {
         // if this context was created from findAll():
         allsets = getAllRRsets();
         for_each(allsets->begin(), allsets->end(), ...);
     }
 };

 class DefaultContext {
 protected:
     virtual vector<ConstRRset>* getAllRRsets() const {
          return (all_sets_);
     }

 private:
     vector<ConstRRsets> all_sets_;
 };

 class InMemoryContext {
 protected:
     // it fully implements getAdditional() internally.

     virtual vector<ConstRRset>* getAllRRsets() const {
          return (NULL);
     }
 };
 }}}

 Regarding this, my suggestion is to keep it for now, and see how
 easy/difficult to implement the customized in-memory version, and then
 think about better reorganization based on the experience.

 > The python interface is not changed. Is it planned to any other ticket?
 Because it would not be possible to call the getAdditional, etc, from
 python right now.

 I was aware of that, too (but I should've mentioned it).  Right now
 I'm not sure if we want to provide python interface for these.  The
 context class and its methods are basically intended for allowing
 actual data source implementation to introduce customized (often
 optimized) behavior.  The services available via the context can be
 achieved using the existing ZoneFinder() interface.  So, right now, I
 don't see much need for the python wrapper of the context interface.

 On the other hand, in terms of completeness it's generally better to
 provide the same interface for both C++ and python.

 Maybe we should just create a ticket mentioning the possibility so we
 can revisit the point later.   If we see as stronger need at that
 point, we can add it; if not, maybe that indicates it's unlikely that
 python apps need it and we can forget it for a relatively longer term.

 > Also, could a test check that the vector passed to getAdditional is not
 cleaned by it, that is, the data are just appended? I think all tests are
 done with empty vector now.

 Good point, added it.

 > I noticed an indentation problem here:
 > {{{#!diff
 > @@ -821,17 +826,17 @@ DatabaseClient::Finder::findNoNameResult(const
 Name& name, const RRType& type,
 >              arg(accessor_->getDBName()).arg(name);
 >          const ConstRRsetPtr nsec = dnssec_data ? findNSECCover(name) :
 >              ConstRRsetPtr();
 > -        return (FindResult(NXRRSET, nsec,
 > -                           nsec ? RESULT_NSEC_SIGNED :
 RESULT_DEFAULT));
 > +        return (ResultContext(NXRRSET, nsec,
 > +                        nsec ? RESULT_NSEC_SIGNED : RESULT_DEFAULT));
 >      } else if ((options & NO_WILDCARD) == 0) {
 > }}}

 Thanks, fixed.

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