BIND 10 #1688: duplicate RR suprression in auth::Query

[BIND 10 Development]

#1688: duplicate RR suprression in auth::Query
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:18 jelte]:

 > I must say I also didn't see the use of an extra class for this; the
 complexity should be the same if we use a local helper method instead of
 direct push_back for the three vectors in the Query class (which would
 only add to the given vector if they are not present in any). The only use
 this has is that now, it can never happen that rrsets are not put in
 answer if they are already present in authority or additional (whereas a
 local checker would result in first-come-first-serve instead of
 prioritizing answer>auth>additional)

 I'm not sure if I understand the local method approach...at least I
 interpreted it as you didn't request a change to the branch by saying
 this, so I didn't do anything on the code for this part.  If you
 suggested a specific change, please say so (and clarify the
 suggestion:-).

 > comments on the code itself:

 > * ResponseCreator::create(): additionals argument is not a reference

 Ah, good catch.  Fixed.

 > * new includes in rbnode_rrset_unittests.cc? (<algorithm> and <sstream>,
 I suspect these are leftovers from now-removed code)

 algorithm really doesn't seem to be necessary, so I removed it.
 sstream does actually seem to be necessary because we use
 ostringstream (although it's not an issue introduced in this task).
 So I kept it.

 I also used this opportunity to align the order of header files based
 on our recent consensus.

 > * leftover newline in rbnode_rrset.cc

 Removed.

 > * there are new includes in rrset_unittest too (iostream)

 Right, iostream doesn't seem to be necessary, but like
 rbnode_rrset_unittests, technically we'd need sstream.  So I added.

 Also reordered the header files.

 Finally, there was one more thing I intended to make (but forgot to do
 before asking for review), although it was an unrelated cleanup: see
 953d4bc.

 > Oh and I suggest the changelog entry does not do 'used-to'; i.e. make it
 something like "b10-auth now filters out duplicate rrsets when building a
 response message" (can still say what it used to do, as long as we
 explicitly say what it does now :))

 Okay, and I realized we should clarify it's for the new query logic.

 {{{
 407.?   [bug]           stephen, jinmei
         b10-auth now filters out duplicate RRsets when building a
         response message using the new query handling logic.  It's
         currently only used with the in-memory data source, but will
         also be used for others soon.
         (Trac #1688, git TBD)
 }}}

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