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

[BIND 10 Development]

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

Comment (by jinmei):

 Replying to [comment:12 naokikambe]:

 > > I can't understand the point of the things defined in
 > > statistics/counter.py.  Defining a common base class for counters
 > > is probably a good idea, but I don't understand what it buys
 > > with the convoluted trick of `_COUNTER` and `init()`.  After all,
 > > most of xfrout-specific work is implemented in `XfroutCounter`,
 > > so it seems to be sufficient if we just use a straightforward mapping
 > > (dict) from a counter name to value.
 >
 > ``_COUNTER`` and ``init()`` are used for initialized counters of
 b10-xfrout.
 >
 > In the future, I planned to put common codes into the parent class.  In
 #2300, which is unreviewed ticket, I tried to put most of common codes
 into the parent class as much as possible.

 In that case I guess we should begin with that version of base class.
 Right now, the base class just seems to be a redundant abstraction,
 just making the architecture non understandable without any benefit.

 > > I'm also concerned about the singleton nature of the counter object.
 > > IMO, a singleton is generally a source of all problems (very
 > > unfriendly with tests to name one) and should be avoided unless
 > > absolutely necessary.  Since I don't see the need for the trick, I
 > > don't understand why it's absolutely necessary either.
 >
 > For example, b10-xfrout should walk with having the counters in each
 library, e.g. notify_out.py. If the target module creates the counters
 once, then it can use them in such a library without any further creation.
 So the counters have to be statically implemented and created. If not,
 multiple instances of the same counter may be created and it may become
 difficult to keep consistency between the values of them.

 Maybe I don't fully understand it, but, to me, defining a things like
 statistics counters at the module level doesn't seem to be a good idea
 in the first place.  It looks more reasonable to me if it's composed
 in some other class object, such as `XfroutServer`.

 > > In any case, it's very awkward to see things like inc_notifyoutv4()
 > > and `XfroutCounter` are defined in the generic counter module.  It
 > > should be defined somewhere else, somewhere more specific to the
 > > relevant module(s).  Same sense of comment applies to the module
 > > description.  Using some specific module as an example is probably
 > > good to help understand it, but I think the general description should
 > > be independent from a specific module.
 >
 > noftify_out.py can be imported in modules other than xfrout.py like
 zonemgr.py. But in such a module, such counters doesn't need to be used.
 Because it doesn't either import or initialize them.

 Here we seem to be talking about different issues.  I simply said it
 looks strange that the module named isc.statistics.counter (which is
 even independent from DNS, much less xfr or notify) contains something
 specific to "notify" or "xfr".  Wherever the actual place `XfroutCounter`
 is defined, it at least shouldn't be in isc.statistics.counter.

 According to your plan, the base counter class may contain more logic
 that is currently implemented in `XfroutCounter`.  Depending on
 whether the resulting base class is a clean abstraction, it may or may
 not make sense.  So, I again, I suggest we begin with that
 abstraction, i.e, define a more meaningful base Counter class, and
 shows how xfr can use it module-independent manner.

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