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):

 It basically looks clean and okay.  I have some minor comments for the
 implementation and some general discussions (the latter is not
 necessarily a showstopper).

 '''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?
 - This module doesn't do any logging.  Is it okay?  Note that I think
   there's at least one event we would like to log (see below)
 - 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?
 - About the magic number of 100: maybe we should define it via a
   constant variable with some comments on the rationale (actually it
   was simply derived from BIND 9's implementation and we don't know
   how it was chosen).
 {{{
 # explain what this is and how we choose the value
 DIFF_APPLY_THRESHOLD = 100
 }}}

 '''diff.py'''

 - Diff.__init__(): the argument name "datasource" might be a bit
   misleading, because it's actually a datasource "client".

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

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

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

 - Diff.compact(): Related to the previous point, should we
   also take into account the RR class? (i.e., check the RR class and
   don't combine them if the classes are different)...probably not in
   any case, though: If we assume the RR class is valid, we should rely
   on the assumption here; if we have add()/remove() check the classes,
   compact() can rely on the result.

 - Diff.apply(): maybe we should make the object unusable (e.g. by
   adding a flag) when it sees an exception from the data source in
   add_rrset(), remove_rrset() and commit(), so that even a careless
   application cannot accidentally continue the operation?  Not a
   strong opinion, maybe noting we should stop using it when that
   happens, just a thought.

 '''diff_tests.py'''
 - __data_common(): argument 'name' is a bit confusing as it may sound
   like a domain name, I'd rename it 'operation', 'op', etc.
 - __data_common(): a minor point, but technically, "add" may not be
   100% correct because it may be remove:
 {{{
         # Add some proper data
         method(self.__rrset1)
         method(self.__rrset2)
 }}}

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