BIND 10 #1259: framework for adding/deleting RR in datasource from xfrin

[BIND 10 Development]

#1259: framework for adding/deleting RR in datasource from xfrin
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20111011
  blocker                            |            Resolution:
                  Component:  xfrin  |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:5 jinmei]:
 > '''general'''
 > - This is not directly related to this branch, but why does it import
 >   isc.dns, not pydnspp?  Or, why do we have both isc.dns (as a
 >   forwarder module) and pydnspp in the first place?  Maybe we should
 >   deprecate pydnspp and unify them into isc.dns?

 I don't know. I just looked into some module around and found it is using
 isc.dns, so I used that too. It looks nicer to me, so I'm in favor of
 having just one name for the module, isc.dns is more consistent.

 But if I should use pydnspp, I'm OK with that of course. Should I replace
 it?

 > - Is the module name 'xfrin' appropriate?  I guess we'll reuse (whole
 >   or part) of this code for dynamic update.  We may also want to
 >   support other ways to change zone data (e.g. via bindctl), and it
 >   may also want to use this module.  Perhaps we should eventually have
 >   a single isc.dns module (package) and make this module a sub module
 >   of it, like isc.dns.diff?

 I'm not sure. I just needed some module and didn't want to spend too much
 time on deciding about the name, when we need it now. If you have a better
 name, I'll switch it.

 > - Diff.__init__(): it creates an updater at initialization time.  This
 >   is slightly different from the original BIND 9 behavior.  In BIND 9,
 >   we delay starting the transaction when first apply() happens,
 >   thereby limiting the duration of the transaction even if the remote
 >   server is slowly feeding the data.  If you thought that would be a
 >   premature optimization and intentionally skipped that, I'm okay with
 >   it; just checking the intent.

 I don't know if it optimises anything at all, but that's not the reason
 why I put it directly to the constructor. I want to raise errors (like
 non-existant zone or the data source not implementing updates) as soon as
 possible. I'd either need to ask for the zone ID and delay the real
 creation for later, but that one looks more complicated and more work for
 computer as well, so I just did it this way.

 > - Diff.compact(): in a future version we should probably treat
 >   RRSIGs that cover different RR types separately, just as BIND 9
 >   does.  I'm okay with deferring it as in practice we won't have to
 >   worry about the case, and in any event that wouldn't matter in our
 >   current sqlite3 schema (and it's the only writable data source for
 >   now).

 ACK, let's leave it for later.

 > - Diff.compact(): BIND 9 checks if the TTLs of the combined RRs are
 >   the same, and a warning log if they aren't.  We should probably do
 >   the same.  (and we need a test for that case)

 I added the check and test. I'm not sure what prefix I should give to the
 logging messages, it may depend if we want to rename the library to
 anything.

 > - Diff.add(), remove(): maybe we should check if the RR class is
 >   identical to that associated with the client (updater).
 >   hmm, BIND 9 doesn't check this at all, maybe trusting the caller.
 >   I know in BIND 9 the DNS message parser ensures all RR classes are
 >   the same, so it probably relies on that check.  Either way is okay
 >   for me - but if we assume the RR class is valid within this module,
 >   this should be documented for the class pydoc.

 OK, the check was added.

 Thanks

-- 
Ticket URL: <https://bind10.isc.org/ticket/1259#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development