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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 > > '''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?

 No, I don't think so.  This is a larger topic, and not only for this
 module anyway.  Let's discuss it separately (outside of this ticket).

 > > - 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.

 One possibility would be making it a submodule of the datasrc package:
 isc.datasrc.diff (on second thought it would at least be better than a
 submodule of isc.dns (or pydnspp) as the diff module depends on the
 concept of datasrc).  But it's not a well-baked thought, just an idea.
 If you like it, please feel free to use it.  If you don't or simply am
 not sure, I'm okay with keeping the current module name.

 > > - 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.

 In practice I suspect these errors normally don't occur in the context
 where this class is used: the application (e.g. xfrin or dyn-update
 handler) should have confirmed the zone exists in the corresponding
 data source, and I guess the writability is ensured via the
 configurations for these apps.

 As for optimization, maybe it's not much.  As long as the application
 creates the Diff object on receiving a first IXFR response (or the
 dynamic update request), the saved duration of transaction is just for
 building the data locally (rather than the one involving network
 delay), which would be negligible.

 In the end, if you think the current way makes the implementation
 simpler, I'm okay with it.

 I have a couple of more comments about the revised code:
 - I'd add to the TTL log message something similar to BIND 9's log
   "adjusting %lu -> %lu".  I'd also explain which TTL is overridden in
   the detailed version of the log description.
 - Related to this point, I'll test the resulting TTL as part of
   test_ttl (maybe for both cases of first TTL > second and first <
   second to see it's just about the ordering, not the
   smallest/largest)

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