BIND 10 #1177: NSEC support in new data source

[BIND 10 Development]

#1177: NSEC support in new data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110927
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by vorner):

 Hello

 So, answering the first part first, the comments are long.

 Replying to [comment:7 jinmei]:
 > - as a result of replacing getRRset() with this one, most
 >   of the documentation for the original method was removed, which
 >   doesn't seem to be good (even though it may not be fully detailed as
 >   it's private).

 Yes, good catch, I intended to adapt it and forgot about it.

 > - I'm not sure if this comment is relevant, but at least NSEC should
 >   be able to coexist with NS/CNAME (and as commented DS should be okay
 >   with NS, too):

 I added the NSEC and NSEC3 (I expect that one is there too). The DS needs
 separate
 bool variable, as it is not allowed with CNAME.

 > - regarding the previous point, doesn't it make more sense to move
 >   this type of check outside of this method completely?

 From the design point of view, maybe. But this is the place where it seem
 easiest. If we were to take them out, all the NS and CNAME and other types
 would have to be returned every time and every time a check would have to
 be performed, which looks like a bad idea.

 > - A map<RRType, RRsetPtr> object is copied twice: when constructing
 >   FoundRRsets at the end of the method, and when passing the return
 >   value to the caller.  Unlike the previous type (a shared pointer),
 >   the copy overhead is bigger.  Should we care about it?  (perhaps
 >   doesn't matter much, because operations requiring DB access are
 >   generally slow anyway).

 As the map will contain only few elements, it shouldn't be much of a
 problem. And I don't think it is unrealistic to expect the compiler to
 optimise both copies away and create the result in-place, as the complete
 code of map is available.

 > - empty_types is defined as a namespace scope static variable, which
 >   seems to be dangerous in terms of static initialization fiasco.
 >   Looking into how it's used, I suspect we actually don't
 >   (necessarily) need it, especially with taking a risk of introducing
 >   the fiasco:
 > {{{
 >     //static WantedTypes nsec_types(empty_types + RRType::NSEC());
 >     static WantedTypes nsec_types(WantedTypes() + RRType::NSEC());
 > }}}

 Ah, right. Well, I don't think there could be the fiasco, but from second
 look at it, it was probably mixture of premature optimisation on my side
 and confusion about what I'm going to write anyway.

 > - operator+: I personally don't think this definition is intuitive.
 >   I'd expect an 'operator+' (with two params) to take the same types
 >   of arguments and to meet other natural expectations derived from the
 >   arithmetic '+' operator (such as the commutative law).  Also, while
 >   admittedly this may purely be a matter of taste, I generally agree
 >   with the concerns documented in the google's guideline:
 >   http://google-
 styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overloading

 Actually, I don't agree there at all with the google styleguide. Then
 we're getting to the java world, where you have to write
 string_a.equals(string_b), because it can't overload operators. That is
 much less readable code than with ==.

 As with this concrete operator ‒ adding an element to a set is something I
 did see written this way in mathematics, mainly in graph theory and
 combinatorics (and even substracting an element from set). So, to me, the
 use of the operator for this is much more intuitive than + with strings.
 You have a valid point with the commutative thing, but it isn't used the
 other way around (in the code, nor in mathematics), even if it could be,
 so I just saved myself writing another copy with switched order of
 parameters.

 Even if the definition might be less intuitive, I think the intention is
 clear where used and probably can't be confused with any other possible
 use of the + operator.

 > - Considering these, I'd suggest something like this:
 >   - Define nsec_types etc via a proxy function:
 > {{{
 > const set<RRType>&
 > NSEC_TYPES() {
 >     static set<RRType>* types;
 >     if (types == NULL) {
 >         auto_ptr<set<RRType> > types_ptr(new set<RRType>());
 >         types_ptr->insert(RRType::NSEC());
 >         types = types_ptr.release();
 >     }
 >     return (*types);
 > }

 That's what I tried before introducing the operator and it turned out to
 be a lot of typing and quite a lot of code, which was much less readable
 than simple inline list of types separated by + signs.

 So, I'd rather like to keep the operator there. It's pity we can't
 overload the , operator, because that one would be even better for the
 list. But this is C++, not perl6.

 > - the changes in the first for loop is not a straightforward
 >   refactoring of the original code, and it's difficult to be sure if
 >   it really preserves the original behavior.  For example, is the
 >   behavior same when DNAME exists and the first 'if' conditions are met?

 Well, as I get it, that is the exact reason why we have tests. All the
 previous tests did pass, so the behaviour we are interested in should be
 the same. And even when I decided the original code was too complicated
 and I just rewrote this bit and it potentially acted differently in some
 situation than the original, if this one looks correct (and it does to
 me), it should be OK, because we did not try the original in anything but
 the unittests anyway.

 So, is there a reason to stick to the exact original code?

 > - as a more fundamental point, this method is now generally too big
 >   and complicated, and difficult to follow (although it's not a result
 >   of this branch).  I suspect we'll need overall cleanup/refacotring
 >   to improve clarity and readability.  (does #1198 intend to improve
 >   the situation?)

 Yes, that's the exact reason for #1198, you are not the first one to
 notice ;-).

 > - (ditto) this "TODO" now seems to be doable:

 Yes, right, I modified that one. That one solves the star thing as well.

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