Opened 8 years ago

Closed 8 years ago

#1371 closed task (complete)

IXFR-out protocol handling: normal case

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20111122
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 6.52 Internal?: no

Description

normal = the requested range of diff is stored in the data source.

Subtickets

Change History (14)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:2 Changed 8 years ago by jelte

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

comment:3 Changed 8 years ago by jinmei

If someone wants I think it's possible start this work based on the
current snapshot of trac1372.

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

trac1371 is ready for review.

It's based on a snapshot version of #1372. The first commit is a
merge for the dependency and should be ignored for review.
While #1372 itself is now being reviewed, I believe the result won't
affect this branch so this one can be reviewed separately.

The plan is that when both #1371 and #1372 are completed, merge the
latter to the former, and merge it to master.

The proposed changelog entry:

326.?	[func]		jinmei
	b10-xfrout now supports IXFR.  (Right now there is no user
	configurable parameter about this feature; b10-xfrout will
	always respond to IXFR requests according to RFC1995).
	(Trac #1371 and #1372, git TBD)

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

I like how it turned out to work the same way for IXFR and AXFR.

There are two things I noticed:

  • The create_soa and other create_* functions are twice in the source code, once in the creatediff.py, once xfrout_test.py. Would it be possible to have them in some common place just once?
  • The test test_reply_xfrout_query_ixfr checks only length of the answer? Is it enough? I noticed that generally the tests don't examine much about which RRs are there, only look at specific SOA or the count only.

Thank you

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

Replying to vorner:

Again, thanks for the prompt review.

I like how it turned out to work the same way for IXFR and AXFR.

There are two things I noticed:

  • The create_soa and other create_* functions are twice in the source code, once in the creatediff.py, once xfrout_test.py. Would it be possible to have them in some common place just once?

Yeah, I had that idea, too. I was not sure if it made sense in this
case as creatediff was mostly just for reference, but now is probably
good time to do it. It required a bit organizational change
(introduce a new test-only module), but I did it.

  • The test test_reply_xfrout_query_ixfr checks only length of the answer? Is it enough? I noticed that generally the tests don't examine much about which RRs are there, only look at specific SOA or the count only.

You're right, it's better to examine more data. I was aware of that,
but was a bit lazy to do this without either bigger organizational
change or copy-paste utility functions. Again, now pointed it out,
I did it with a bit bigger organizational change.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

It looks OK, just this one thing. The comment in rrsets_equal seems incomplete, the sentence doesn't seem to have a werb:

+
+    # no accessor for sigs either (so this only checks name, class, type, ttl,
+    # and rdata)
+    # also, because of the fake data in rrsigs, if the type is rrsig, the
+    # rdata is not checked

Otherwise it seems OK to merge.

Thank you

comment:12 Changed 8 years ago by vorner

  • Total Hours changed from 0 to 1

comment:13 in reply to: ↑ 11 Changed 8 years ago by jinmei

Replying to vorner:

It looks OK, just this one thing. The comment in rrsets_equal seems incomplete, the sentence doesn't seem to have a werb:

+
+    # no accessor for sigs either (so this only checks name, class, type, ttl,
+    # and rdata)
+    # also, because of the fake data in rrsigs, if the type is rrsig, the
+    # rdata is not checked

Ah, that was actually a "complete" copy of the original implementation
in isc.datasrc.tests, but the comment didn't make sense without the
original description comment for the function. I've revised the
entire docstring/comment to:

    '''Compare two RRsets, return True if equal, otherwise False

    We provide this function as part of test utils as we have no direct rrset
    comparison atm.  There's no accessor for sigs either (so this only checks
    name, class, type, ttl, and rdata).
    Also, since we often use fake data in RRSIGs, RRSIG RDATA are not checked.

    '''

Otherwise it seems OK to merge.

Okay, thanks. I'll merge trac1372 to this and then to master, and
close the tickets.

comment:14 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 1 to 6.52
Note: See TracTickets for help on using tickets.