Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1287 closed task (complete)

ZoneIterator will need an internal finder

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20111108
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: 9 Add Hours to Ticket: 0
Total Hours: 9:00 Internal?: no

Description

I suspect the ZoneIterator? class will need to provide an interface
of an "internal ZoneFinder?" just like the ZoneUpdater? class for the
AXFR support: We need to get the SOA RR of the zone before iterating
over the zone (and in general we wouldn't be able to assume we can
get the SOA from the interator soon enough).

In the database version of the implementation, both need to refer to
the same version in terms of database updates, so the ZoneIterator? will
have to internally create a transaction. It will also have to be clone()'ed
from the original accessor (like ZoneUpdater?).

Subtickets

Change History (15)

comment:1 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Year 3 Task Backlog

comment:2 Changed 8 years ago by vorner

  • Milestone changed from Year 3 Task Backlog to Sprint-20111025
  • Owner set to vorner
  • Status changed from new to accepted

Missing dep, taking it into the sprint.

comment:3 Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to assigned

I don't have time soon for this, so I'm giving it back and someone cat get it.

Note that there's already one commit on the branch, but it's quite straightforward, defining the interface and adding a documentation to it. No tests or code for it exist.

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by jinmei

trac1287 is ready for review.

I made one significant difference from the original plan: instead of
introducing a generic "finder", I simply added a more focused "getSOA()"
method. I found that the generic behavior of the finder would be
very difficult to implement (probably impossible) for some types of
databases if we want to have both the finder operations and iterations
in the same single transaction. From my quick experiment, at least
it did't seem to be possible with MySQL in a straightforward way.

In practice, all we'd need to know is zone's SOA as a shortcut, and
that should be much easier to implement: we can simply find it before
starting the iteration, and return that record when asked.

I needed to tweak the test cases and mock accessor to test the various cases,
but the main code should be pretty straightforward: it just clones a
separate accessor, starts a transaction, finds an SOA and remembers it,
then start iteration. the new getSOA() method simply returns the remembered
value.

The python wrapper code should also be straightforward. While we should
add the same level of tests as C++, I'd defer it to a separate task
(we already have a ticket for this). I'd also defer the implementation
of getSOA() for the in memory data source to a separate ticket to keep
the diff minimum (and we won't need it for in-memory for the moment
anyway).

I'm not planning to add a new changelog entry, but since this is a
new API, it may make sense to give it. I'm okay with either way.

comment:7 Changed 8 years ago by jinmei

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

comment:8 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

Just some minor things:

  • This is probably rest of the one commit of mine, but I guess it's no longer needed (in database.cc):
    +
    +    virtual ZoneFinder& getFinder() {
    +        isc_throw(NotImplemented, "Not implemented");
    +    }
    
  • I guess this is never called multiple times? Otherwise the copy will be done to itself. Would that be OK?
    virtual void startTransaction() {
            // Currently we only use this transaction for simple read-only
            // operations.  So we just make a local copy of the data (we don't
            // care about what happens after commit() or rollback()).
            readonly_records_copy_ = *readonly_records_;
            readonly_records_ = &readonly_records_copy_;
    }
    
  • A typo: updateAfterDelteIterator

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

Replying to vorner:

Thanks for the review.

Just some minor things:

  • This is probably rest of the one commit of mine, but I guess it's no longer needed (in database.cc):
    +
    +    virtual ZoneFinder& getFinder() {
    +        isc_throw(NotImplemented, "Not implemented");
    +    }
    

Ah, good catch. Removed.

  • I guess this is never called multiple times? Otherwise the copy will be done to itself. Would that be OK?
    virtual void startTransaction() {
            // Currently we only use this transaction for simple read-only
            // operations.  So we just make a local copy of the data (we don't
            // care about what happens after commit() or rollback()).
            readonly_records_copy_ = *readonly_records_;
            readonly_records_ = &readonly_records_copy_;
    }
    

It wouldn't be okay, and right now, for simplicity, we assume it's called
at most once. I added some note about this and added explicit check code
so that we can notice that more explicilty should this happen with a buggy
test case.

  • A typo: updateAfterDelteIterator

Thanks, fixed.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

OK, seems fine. Please merge.

Thank you

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

Replying to vorner:

OK, seems fine. Please merge.

Okay, thanks, merge done, closing ticket.

comment:14 Changed 8 years ago by jinmei

  • Estimated Difficulty 0 deleted
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 9:00

comment:15 Changed 8 years ago by jelte

  • Estimated Difficulty set to 9
Note: See TracTickets for help on using tickets.