Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#2282 closed task (complete)

implement specialized additional handling in in-memory data source

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20121009
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: 4.63 Internal?: no

Description

We should re-implement specialized additional record handling for the
new in-memory data source implementation.

Specifically, we should implement specialized getAdditionalImpl()
method for the InMemoryZoneFinder::Context class:

  • it should remember the RdataSet returned as the answer
  • use RdataReader to iterate over names in the rdataset that requires additional section processing
  • lookup in the tree for the corresponding LabelSequence and get the requested type RRsets

Subtickets

Change History (12)

comment:1 Changed 8 years ago by jinmei

  • Type changed from defect to task

comment:2 Changed 8 years ago by muks

  • Estimated Difficulty changed from 0 to 4

comment:3 Changed 8 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121009

comment:4 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:5 Changed 8 years ago by jinmei

trac2282 is ready for review.

The diff is reasonably small (I believe), and should be pretty
straightforward. We can check the behavior with the exiting
ZoneFinderContextTest tests, so we don't need to add many tests
within this branch (I added one specific test case).

The first commit is to merge dependency and should be ignored for
review.

May not be very likely, but if possible I'd like to complete #2218 and
this ticket and merge the in-memory implementation before the release
(it's still slower than the current one, but I think the drop will be
in an acceptable range with this feature of this ticket).

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:7 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

First, the same lettuce test as in #2219 fails, but I think that is to be expected, as it is based on the branch.

I noticed one problem with indentation here (the code and zone_data start at different column):

                            const ZoneNode* node, const RdataSet* rdset) :
       code(code_param), rrset(rrset_param), flags(flags_param),
        zone_data(&zone_data_param), found_node(node), found_rdset(rdset)
    {}

Also, can it happen one RRset is returned multiple times? For example, when
there was an ANY query and there's both MX and NS record pointing to the same
name. I don't see anything preventing the second copy to be created and added.
Is it OK and handled on a higher level?

If the answer to the last one is it is solved somewhere in the auth query
processing, I think the branch is ready for merge (with the fix of
indentation).

With regards

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

Replying to vorner:

I noticed one problem with indentation here (the code and zone_data start at different column):

                            const ZoneNode* node, const RdataSet* rdset) :
       code(code_param), rrset(rrset_param), flags(flags_param),
        zone_data(&zone_data_param), found_node(node), found_rdset(rdset)
    {}

Ah, good catch. The code beginning with code has a tab. Replaced it
with spaces, which should fix the issue.

Also, can it happen one RRset is returned multiple times? For example, when
there was an ANY query and there's both MX and NS record pointing to the same
name. I don't see anything preventing the second copy to be created and added.
Is it OK and handled on a higher level?

It's okay. getAdditionalImpl() is not responsible for duplicate check.
It's done in auth's Query class (or whatever user of the context
class). After all, duplicates may exist in other sections, so even if
getAdditionalImpl() checks duplicates for additionals it's not a
complete check anyway.

If the answer to the last one is it is solved somewhere in the auth query
processing, I think the branch is ready for merge (with the fix of
indentation).

Okay, thanks. So we should be almost ready.

comment:10 in reply to: ↑ 9 Changed 8 years ago by jinmei

Replying to jinmei:

If the answer to the last one is it is solved somewhere in the auth query
processing, I think the branch is ready for merge (with the fix of
indentation).

Okay, thanks. So we should be almost ready.

Now merge done with dependency branches. Closing.

comment:11 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 4

comment:12 Changed 8 years ago by vorner

  • Total Hours changed from 4 to 4.63
Note: See TracTickets for help on using tickets.