BIND 10 #1604: reorganize RRset class hierarchy to allow further optimization

[BIND 10 Development]

#1604: reorganize RRset class hierarchy to allow further optimization
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120207
                  Component:         |            Resolution:
  libdns++                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:  auth   |           Total Hours:  0
  performance                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => stephen


Comment:

 Hello

 Replying to [comment:8 stephen]:
 > > First, I'm not sure it is the best thing to do to throw
 !NotImplemented on getRRsig
 > This is only in the BasicRRset class.  The idea here is that as
 BasicRRset knows absolutely nothing about signatures (these are introduced
 in the RRset subclass), if you're referencing one and using a BasicRRset
 you've made a mistake.

 Not necessarily. Imagine a simple datasource backend (let's say the built-
 in one). It makes sense for it to return BasicRRset objects, because these
 RRsets will never be signed. So, from the point of the datasource it makes
 sense this way.

 However, the query processing would want to do something like (not
 examining the full details about correct method names, etc):
 {{{#!c++
 RRsetPtr result = backend->find(parameters);
 message->addRRset(result);
 RRsetPtr rrsig = result->getRRsig();
 if (rrsig) {
         message->addRRset(rrsig);
 }
 }}}

 As the built-in has no signatures, it would expect it to return NULL.
 However, if it throws, it would have to be changed to this to make it
 work:

 {{{#!c++
 RRsetPtr result = backend->find(parameters);
 message->addRRset(result);
 RRsetPtr rrsig;
 try { rrsig = result->getRRsig(); } catch (const isc::NotImplemented&) { }
 if (rrsig) {
         message->addRRset(rrsig);
 }
 }}}

 Which is less convenient and readable and provides no real advantage.

 The thing I say we should do is instead of having
 {{{#!c++
 RRsetPtr getRRsig() const { isc_throw(isc::NotImplemented); }
 }}}
 to change it to
 {{{#!c++
 RRsetPtr getRRsig() const { return RRsetPtr(); }
 }}}

 This doesn't require the BasicRRset to know anything about RRsigs except
 that it doesn't know anything about them.

 > > With the addRRsig methods, would it be better to give the parameter
 (like {{{rdata::ConstRdataPtr}}}) by reference?
 > This is a point of discussion between myself and Jinmei - see the
 comments in #1305.  It has not been fully resolved but currently no change
 is being made.  (Having said that, where a {{{ConstXxxxPtr}}} is not being
 modified, passing the argument as {{{const ConstXxxxPtr&}}} might overcome
 Jinmei's objections.)

 Yes, that's what I mean, if it is not modified, there's no need to pass it
 as value. The concern of storing it as a reference somewhere else applies
 even if it is passed as a parameter, you can take a reference of that one
 as well.

 > > I don't know if there's much we can do about it, but the part with
 Associated RRsig methods in BasicRRset looks too much like a copy-paste,
 including the documentation. I fear that if we ever need to change
 anything there, we'll forget to change all the copies and we'll get
 inconsistency.
 > True, but at the moment if we don't copy the information, Doxygen will
 complain.

 That's what I say about doxygen being broken with regards with the
 warnings. These methods should be documented as a group only, saying they
 are not implementing and they throw so. Also, when reading 3 times per one
 paragraph/function that the function just throws NotImplemented is kind of
 2 times too much. The purpose of documentation, in my opinion, is to save
 the user of API some reading of code. But if the documentation is
 significantly longer than the code, it is less work to read the code than
 the documentation loses its point.

 > One other thing: suggested text for the !ChangeLog is
 > {{{
 > Alter RRsetPtr and ConstRRsetPtr to point to AbstractRRset (instead of
 RRset)
 > to allow for specialised implementations of RRsets in different data
 sources.
 > }}}

 Yes, this one looks OK. But I think it should be the „[func]*“ category
 (backward incompatible).

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