BIND 10 #2018: Use updater's finder in DDNS

[BIND 10 Development]

#2018: Use updater's finder in DDNS
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120612
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:8 jinmei]:
 > Replying to [comment:7 jelte]:
 >
 > > > - exposing the updater seems to make the integrity of the Diff class
 > > >   fragile.  [...]
 > >
 > > That second approach seems nice, but I kind of prefer the first one,
 not really for now, but we may want to consider to have these find methods
 return the data not directly from the updater's find() and find_all(), but
 including the changes made in the diff so far. This would be a nontrivial
 task, but could make the calling side (related to #2016) much cleaner. A
 disadvantage would be that the find calls in diff and the add/delete calls
 would need to be way smarter than the simple things they are now. Not
 quite as smart as the real finds (since this should not need additional
 processing, and only work on raw data), but still, something to keep in
 mind.
 > >
 > > For now I'm just wrapping the find() and find_all() calls.
 >
 > Okay, and this reminds me of another thing.  For the "smarter"
 > behavior, I guess another approach is
 >
 > - ddns (session) can make changes in the update section one by one,
 >   without doing any buffering
 > - if this processing needs to examine the latest snapshot of the
 >   updated zone (before commit), ddns (session) can do it with the
 >   updater's finder.
 > - extend the datasrc API in order to allow installing diffs only
 >   (separately from the corresponding changes to the zone)
 > - ddns (session) normalize the entire changes into the IXFR-friendly
 >   form of diffs, and install it using the extended API, just before
 >   committing.
 > - ddns (session) finally commits the changes (with the diff update)
 >
 > This is similar to BIND 9's behavior.  Of course, that's beyond the
 > scope of this ticket in any case.
 >

 That is another reasonable approach, I have some doubts about whether it
 would be much better though; I suspect (note, not 100% sure) that in
 essence, to create the diff, you'll have to do nearly everything we now
 have to do anyway (even if you update directly), and I would be worried
 that the locally built diff could get out of sync with what has actually
 been sent to the updater. So in order to have that reasonably, i'd think
 we'd actually get a quite similar abstraction as the current Diff to
 handle that.

 *ideally* i still think it would be best if the zoneupdater would just
 take anything in any order and keep track of changes itself :P

 BTW I am starting to think that maybe we should not have reused
 xfrin.diff.Diff for these single-updates; nearly every operation either
 needs a different handler or it is only valid for one type. Thinking of
 splitting them up into two classes, with perhaps a base class for some of
 the internals (like setting up the updater).

 >
 > In conclusion I don't oppose to this change, but as I suggested in
 > bind10-dev, I think we should do it with documented clarification.
 > We should also add a test that ACL is really checked before
 > prerequisite (the fact that you can simply change the ordering seems
 > to suggest we currently don't have that type of test).
 >
 > But, again, these will be beyond the scope of this ticket.  My
 > personal suggestion is to keep the current (RFC-described) ordering
 > just in this task, and defer this clarification with doc to a separate
 > task.  But, assuming that no one will object to the eventual behavior
 > of BIND 10, if you really want to make this change now, I don't oppose
 > to that.
 >

 ok, moving it back down :)

 >
 > Some other points to the changes:
 >
 > '''session.py'''
 >
 > - maybe we can define a shortcut for
 >   `ZoneFinder.NO_WILDCARD | ZoneFinder.FIND_GLUE_OK` with the rationale
 >   and use it throughout the module?  Alternatively, we might even make
 >  it the default of Diff.find() and find_all().
 > {{{#!python
 >         result, _, _ = self.__diff.find(rrset.get_name(),
 rrset.get_type(),
 >                                         ZoneFinder.NO_WILDCARD |
 >                                         ZoneFinder.FIND_GLUE_OK)
 > }}}
 >

 In fact, I was considering whether there are ever any other flags you'd
 want in this context, in which case we could just hardcode them in diff
 and leave out the option altogether. There is a reason for this; I am also
 working on 2016 now and after a few quick experiments with approaches I
 think that the 'find_but_modify_result_with_diff_so_far' might not be that
 hard, *provided* the find is always on 'raw' data without additional
 processing

 Made them default for now, might remove the argument completely.

 > '''diff.py'''
 >
 > - in find/find_all() description, this is not 100% correct:
 > {{{#!python
 >         Diff's ZoneUpdater, i.e. the find() on the zone as it was on the
 >         moment this Diff object got created.
 > }}}
 >   if there is any call to apply(), these find variants can find the
 >   applied changes.
 > - we should probably do the `__check_commited()` check for these
 >   methods, too.

 ack, done

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