Opened 8 years ago

Closed 8 years ago

#1791 closed task (complete)

update InMemoryZoneFinder::load() to support sqlite3 backend

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120417
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: xfr for in-memory
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 13.5 Internal?: no

Description

This is a subtask of #1787.

In this task we add another signature to InMemoryZoneFinder::load().
it takes ZoneIterator(Ptr) instead of string& (specifying a file name)
and load the RRsets from the iterator to in-memory.

Note that the in-memory implementation requires some limitation about
how the RRsets are coming (e.g., an RRset must not be divided;
RRSIGs must come after the covered RRset, etc). So the loader may
need some smartness such as doing intermediate buffering.

Subtickets

Change History (15)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Sprint-20120417

comment:3 Changed 8 years ago by jinmei

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

comment:4 Changed 8 years ago by jinmei

trac1791 is ready for review.

The first commit is a setup for subsequent tests. The amount of
change may look a bit large, but it's essentially a trivial
refactoring that moves code from zone_finder_context_unittest to the
new file.

I also made a not-directly-related bug fix (b446618). see the commit
log and change.

Other than that it should be pretty straightforward implementation of
what is stated in the ticket.

Due to the bug fix I think we need a changelog entry (for that
specific part):

422.?	[bug]		jinmei
	The database based zone iterator now separates RRSIGs of the same
	name and type but for different covered types.
	(part of Trac #1791, git b4466188150a50872bc3c426242bc7bba4c5f38d)

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by kevin_tes

  • Owner changed from UnAssigned to kevin_tes

comment:7 follow-up: Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to jinmei

Hi,
I have check the changed codes twice, and it seems good to me.

But I am not sure a detail below:
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc

+        const ConstRdataPtr rdata_base =
+            rdata::createRdata(rtype, class_, rdata_);
+        ConstRdataPtr rdata;
+        while (data_ready_) {
+            bool same_type = true;
+            if (rdata) { // for subsequent data, replace it with the new RDATA.
+                const RRType next_rtype(rtype_);
+                rdata = rdata::createRdata(next_rtype, class_, rdata_);
+                same_type = isSameType(rtype, rdata_base, next_rtype, rdata);

The 'rdata' declares without definition,but it was used in the 'if(rdata)',i have no idea about this.
Otherwise,it can be merged.

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

Replying to kevin_tes:

I have check the changed codes twice, and it seems good to me.

Thanks for the review.

But I am not sure a detail below:
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc

+        const ConstRdataPtr rdata_base =
+            rdata::createRdata(rtype, class_, rdata_);
+        ConstRdataPtr rdata;
+        while (data_ready_) {
+            bool same_type = true;
+            if (rdata) { // for subsequent data, replace it with the new RDATA.
+                const RRType next_rtype(rtype_);
+                rdata = rdata::createRdata(next_rtype, class_, rdata_);
+                same_type = isSameType(rtype, rdata_base, next_rtype, rdata);

The 'rdata' declares without definition,but it was used in the 'if(rdata)',i have no idea about this.
Otherwise,it can be merged.

rdata is defined in line 3 in the above quoted block.

But I guess the more fundamental issue is the (un)readability of this
method. Although I'd say it was unreadable even before my changes in
the branch, I see it could easily confuse readers.

I tried to cleanup the code with more comments so it would be less
confusing. And, while doing so, I found another bug in the
implementation (which existed before this branch), so I fixed it, too.

I'd propose adding another separate changelog entry for this part of
bug fix.

423.?	[bug]		jinmei
	The database based zone iterator now correctly resets mixed TTLs
	of the same RRset (when that happens) to the lowest one.  The
	previous implementation could miss lower ones if it appears in a
	later part of the RRset.
	(part of Trac #1791, git f1f0bc00441057e7050241415ee0367a09c35032)

Could you check the latest branch to confirm the bug fix and see if
it's generally okay now?

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to kevin_tes

comment:10 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to jinmei

Replying to jinmei:

Replying to kevin_tes:

I have check the changed codes twice, and it seems good to me.

Thanks for the review.

But I am not sure a detail below:
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc

+        const ConstRdataPtr rdata_base =
+            rdata::createRdata(rtype, class_, rdata_);
+        ConstRdataPtr rdata;
+        while (data_ready_) {
+            bool same_type = true;
+            if (rdata) { // for subsequent data, replace it with the new RDATA.
+                const RRType next_rtype(rtype_);
+                rdata = rdata::createRdata(next_rtype, class_, rdata_);
+                same_type = isSameType(rtype, rdata_base, next_rtype, rdata);

The 'rdata' declares without definition,but it was used in the 'if(rdata)',i have no idea about this.
Otherwise,it can be merged.

rdata is defined in line 3 in the above quoted block.

But I guess the more fundamental issue is the (un)readability of this
method. Although I'd say it was unreadable even before my changes in
the branch, I see it could easily confuse readers.

I tried to cleanup the code with more comments so it would be less
confusing. And, while doing so, I found another bug in the
implementation (which existed before this branch), so I fixed it, too.

I'd propose adding another separate changelog entry for this part of
bug fix.

423.?	[bug]		jinmei
	The database based zone iterator now correctly resets mixed TTLs
	of the same RRset (when that happens) to the lowest one.  The
	previous implementation could miss lower ones if it appears in a
	later part of the RRset.
	(part of Trac #1791, git f1f0bc00441057e7050241415ee0367a09c35032)

Could you check the latest branch to confirm the bug fix and see if
it's generally okay now?

Yeah, it okay now please merge.:)

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

Replying to kevin_tes:

Could you check the latest branch to confirm the bug fix and see if
it's generally okay now?

Yeah, it okay now please merge.:)

Sorry for bothering you again, but when I tried to merge it I noticed
there was one regression which was introduced in my latest change.

I've fixed it, and added a test that would have found it earlier.

Could you check that once again?

comment:12 Changed 8 years ago by jinmei

  • Owner changed from jinmei to kevin_tes

comment:13 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to jinmei

Replying to jinmei:

Replying to kevin_tes:

Could you check the latest branch to confirm the bug fix and see if
it's generally okay now?

Yeah, it okay now please merge.:)

Sorry for bothering you again, but when I tried to merge it I noticed
there was one regression which was introduced in my latest change.

I've fixed it, and added a test that would have found it earlier.

Could you check that once again?

Check work has done,looks good to me.
Please merge.

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

Replying to kevin_tes:

Could you check that once again?

Check work has done,looks good to me.
Please merge.

Thanks, merge done. Closing.

comment:15 Changed 8 years ago by jinmei

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