Opened 7 years ago

Closed 6 years ago

#2539 closed task (fixed)

implement InMemoryClient iterator getSOA()

Reported by: vorner Owned by: muks
Priority: medium Milestone: bind10-1.2-release-freeze
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: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Currently it throws NotImplemented. This makes the iterator mostly useless
for use in XfrOut? (and we probably don't have other consumer of the iterator
for in-memory data source). Once we have the memory manager working, we will
probably want to use the in-memory in XfrOut? too, since it'll be already loaded
somewhere in the memory and it'll be significantly faster than DB access.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by shane

  • Milestone New Tasks deleted

comment:2 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Owner set to UnAssigned
  • Status changed from new to reviewing

Up for review. There are a few more things to finish before this zone iterator can be used for XfrOut?. See ticket #3289.

comment:3 Changed 6 years ago by muks

No ChangeLog is necessary as this is not used by the end-user (such as for outgoing XFRs) yet.

comment:4 Changed 6 years ago by kean

  • Owner changed from UnAssigned to muks

While this looks OK as it stands, I have a question. Is there any reason not to have a private class member soa_ and initialize that in the constructor, and then simply have getSOA() return that, instead of doing the work currently in getSOA() each time it is called? I don't have a "feel" for how often getSOA() is likely to be called, but that strikes me as being more optimal.

comment:5 Changed 6 years ago by kean

BTW Mukund I made a commit moving the logic you had in getSOA() into the constructor. The unit tests pass. It is in commit 8885b2d4e06fc897eee4efdace356c1bd29c771c. Please feel free to ignore it and delete the commit if this is inappropriate.

comment:6 follow-up: Changed 6 years ago by muks

  • Owner changed from muks to kean

Hi Kean

Is there a use-case for this getSOA() optimization?

The iterator is to be constructed and used where required (it's not a long-living object as it requires the underlying datasource to be constant during its use). The iterator is mainly for use during zone transfers and related serial comparisons. In such places, code that uses it would not have to call getSOA() multiple times. This optimization would further complicate an already complicated iterator for little benefit. It would also break the "identical" guarantee of ZoneIterator::getSOA() (though we assume the underlying data source must not change during iterator use).

But just in case this is required, can you point out a place where getSOA() needs to be called multiple times?

comment:7 in reply to: ↑ 6 Changed 6 years ago by kean

  • Owner changed from kean to muks

Replying to muks:

Is there a use-case for this getSOA() optimization?

As I said in the review, no, I do *NOT*. It was a question, along with a commit framing the question.

It would also break the "identical" guarantee of ZoneIterator::getSOA() (though we assume the underlying data source must not change during iterator use).

How would it do that? It returns the same soa_ each time it is called.

But just in case this is required, can you point out a place where getSOA() needs to be called multiple times?

Again, no I can not, as stated in the review. However, I do try in general to write things not for as their usage is today, but for what it might be in the future (I believe some call that "defensive coding").

Also there is something to be said for consistency. database.cc's implementation of getSOA() does it my way. There may be reasons for that that I am missing, but is there any reason they should be different?

comment:8 Changed 6 years ago by muks

I'm wrong. Upon a re-reading of the ZoneIterator doc, it seems that caching the SOA RRset before zone iteration is actually required. So your change is not only an optimization, but actually necessary.

I'll review the commit and get back to you soon.

comment:9 Changed 6 years ago by muks

  • Owner changed from muks to kean

Hi Kean

This change seems necessary. I have reviewed your commit and while it is fine, it has a bunch of unnecessary assignments which we can remove. I've made a commit towards this. Please review it.

comment:10 Changed 6 years ago by kean

  • Owner changed from kean to muks

This looks fine. Feel free to commit and close.

comment:11 Changed 6 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit 469f80fa1e994479c42898cffb88595cd3ba9185:

* 86baf21 [2539] Removed unnecessary assignments
* 8885b2d [2539] Initialize SOA once during construction rather than in getSOA()
* 48da2e4 [2539] Implement getSOA() in InMemoryClient iterator

Resolving as fixed. Thank you for the reviews.

Note: See TracTickets for help on using tickets.