BIND 10 #2222: Implement counters into Xfrout (2/3)

[BIND 10 Development]

#2222: Implement counters into Xfrout (2/3)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  naokikambe                         |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120925
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  xfrout                             |  Estimated Difficulty:  4
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by naokikambe):

 * owner:  naokikambe => vorner


Comment:

 Hello,

 Replying to [comment:10 vorner]:
 > I'm not sure if I lack python experience, and I'm not at my best today
 > generally. But I find a lot of the code there very hard to read or
 completely
 > unreadable.
 Sorry for the unreadable codes. Please point out if there are other ones.
 I would like to revise as much as possible.

 > These are hard to read. Would it be possible to simplify it somehow? Or
 at
 > least comment what is happening and how?
 > {{{#!python
 >             incrementer = \
 >
 dict(self.xfrout_counter.get_counters_for_xfroutsession(), \
 >
 **self.xfrout_counter.get_counters_for_notifyout())\
 >                          ['counter_%s' % counter_name]
 > }}}
 I revised that code. Please see:
 {{{
 396bb78 [2222] simplified complicated assignment of `incrementer`
 }}}

 > As well as these (I'd suggest using full-blown if-else here):
 > {{{#!python
 >             self._statistics_data[n] \
 >             if n.find('ixfr_') == 0 or n.find('axfr_') == 0 \
 >             else self._statistics_data['zones'][TEST_ZONE_NAME_STR][n]
 > }}}
 I used the full-blown if-else style. Please see:
 {{{
 2fee008 [2222] expanded the lambda statement to the normal method
 definition
 }}}

 > And this is just too much for me, I have no clue what this code does.
 The
 > comment about argument does not help a bit, I have no idea what argument
 it
 > talks about:
 > {{{#!python
 >         # Set counter handlers for counting Xfr requests or
 >         # incrementing or decrementing Xfr running. An argument
 >         # is required for zone name in counting Xfr requests.
 >         for (k, v) in counters.items():
 >             if k.find('counter_') == 0 or k.find('inc_') == 0 \
 >                     or k.find('dec_') == 0:
 >                 setattr(self, "_%s" % k, v)
 > }}}

 I revised the comment. What about this?
 {{{
 7044579 [2222] corrected the comment according to what the code does
 }}}

 > Also, some tests seem to have quite a lot of levarage. I saw similar one
 3
 > times I think. Could the Greater be equal to 1? Because it should
 increase the
 > counter by exactly one transfer:
 > {{{#!python
 >          self.assertEqual(self.get_counter('xfrrej'), 0)
 >          self.check_transfer_acl(acl_setter)
 >          self.assertGreater(self.get_counter('xfrrej'), 0)
 > }}}
 I revised the code to check the counter incremented to one. Please see:
 {{{
 1792477 [2222] changed to check the counter exactly incremented to one
 }}}

 > And, does this test check anything, or is it completely useless? It
 loosely
 > translates to „the counter may or may not have been incremented.
 Whatever the
 > server likes.“
 > {{{
 >     And when I query statistics axfr_running of bind10 module Xfrout
 with cmdctl port 47804
 >     # xfering might not be started yet.
 >     Then the statistics counter axfr_running should be between 0 and 1
 > }}}

 I'm not sure it is completely useless, but I revised the comment:
 {{{
 668cdb3 [2222] updated the comment to be detailed
 }}}

 > And, also, it is mostly aesthetics, but the xxcrementer used at few
 places
 > sounds very much like excrementer, and while I don't think anyone would
 ever use
 > that word in English,

 Thank you for the suggestion. I don't really know that meaning.:-)
 I changed the method name. Please see:
 {{{
 48df8a1 [2222] changed the misleading method name
 }}}

 Regards,

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