BIND 10 #2252: Implement counters into Xfrin (1/3)

[BIND 10 Development]

#2252: Implement counters into Xfrin (1/3)
-------------------------------------+-------------------------------------
            Reporter:  naokikambe    |                        Owner:
                Type:  enhancement   |  jinmei
            Priority:  medium        |                       Status:
           Component:  xfrin         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130423
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:23 naokikambe]:

 > (I would be so happy if such a user used statistics counters much as
 above. But in fact, ..)
 > I think the user probably needs another trigger like `axfrreqv4` or
 `axfrreqv6` or bind10 log or the primary server's log or something.
 Because he/she must realize that the script would not work at first.
 `last_axfr_duration` must keep being 0.0 until the first xfr is done. Also
 in a real case, it would be a very rare that a real xfr is done in less
 than a microsecond. Can we fix the issue on another ticket after this
 ticket? Anyway ..

 This is fine.

 > > In this specific case, completely overriding `ipver` actually creates
 > > an instance variable so the problem wouldn't be apparent.  updating
 > > name_to_counter dict is probably a better example.  But, again, this
 > > is basically a principle matter; the fact that it uses global without
 > > a reason is itself a problem.
 >
 > We cannot cause such a problem as well by updating either
 > > `name_to_counter` or `master_addrinfo` in that case. Because both
 > > of them were defined as a tuple. I'd not like to disagree with

 Ah, okay, that was my misunderstanding.  Perhaps I was too worried
 about the seemingly easy use of global instances including class
 attributes.  In any case, the revised code looks good.

 > > '''xfrin.py.in'''
 > >
 > > - In this revised code, if IXFR results in `XfrinZoneUptodate` xfrfail
 > >   is counted.  Is it intentional, and if so, is it reasonable?  From a
 > >   quick look, BIND 9 seems to consider it a success.  In any case,
 > >   this should be clarified in the documentation: what exactly
 > >   "succeed/fail" mean.
 >
 > I revised the codes to treat an xfr request as success in a case of
 raised `XfrinZoneUptodate`. For consistency between BIND 9 and 10, I think
 we should define this case as success. In fact, `do_xfrin()` already
 defines so. The commit is:
 > {{{
 > 8544c8e [2252] treat an xfr request as success with an XfrinZoneUptodate
 raised
 > }}}

 Documentation update is missing.

 And one minor point: this comment doesn't parse well to me:
 {{{
                 # Note: Catching an exception XfrinZoneUptodate
                 # defines that IXFR succeeded.
 }}}

 And, one last non trivial point.  I'd hesitate to say this after the
 long cycle of review process, but do_xfrin() now looks overly
 complicated to me due to the series of statistics related changes.
 The organization of the nested try-re-raise-catch is particularly
 unreadable to me.  I suggest refactoring this method so we can
 understand the entire flow is more easily.  And, I wouldn't like to
 defer it to a separate ticket; it'll quite likely to be buried in
 other tickets.

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