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

[BIND 10 Development]

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

 * owner:  naokikambe => muks


Comment:

 Hi Mukund san

 Thanks for the comments.

 > * `counter.py` should be renamed to `counters.py`.

 I renamed.
 {{{
 dc7e03f [2225_statistics] rename counter.py to counters.py and make the
 related changes for the review comment
 }}}

 > * Multi-module tests should be added for the various scenarios in which
 the counters of a single spec file will be used, i.e, for cases similar to
 the `notifyoutv4` example (between `b10-xfrout` and `notify_out`) given in
 the `counter.py` module doc, and also for any other cases where such
 common counts will be maintained.

 I added some tests into `counters_test.py`. Is that enough? Please read
 this.
 {{{
 cc0c7a0 [2225_statistics] add multi-module tests for the scenarios
 }}}

 > * We should note in `__concat()`'s documentation that it is a helper
 >   function that is used to generate the keys.
 I revised the note.
 {{{
 08d9023 [2225_statistics] add note to _concat()
 }}}

 > * I think we should remove the step argument from inc() and dec() and
 >   move the code to a common helper method. Exposing step to a caller as
 >   it is done currently is not good code. I have added a comment
 temporarily
 >   to both methods' docs that `step` should not be specified by the
 caller.
 I added `_incdec()` as a helper function. Please see the following for
 details.
 {{{
 78f11bb [2225_statistics] add a common helper function for incrementing or
 decrementing a counter
 }}}

 > * Is `dec()` only meant to be used for `axfr_running` and
 `ixfr_running`?

 That documentation was incorrect. I revised it.
 {{{
 6443e07 [2225_statistics] correct documentation of inc() and dec()
 }}}

 > * This is not a good API doc comment because you are exposing internal
 implementation details to the user of this module which that user may not
 know about (in order to follow the comment). It's simpler to just say
 "Starts a timer and keeps it running until stop_timer() is called.", and
 also talk about any other things that the caller should know. Same for
 `stop_time()`'s API doc too.
 > {{{
 >         """Sets the value returned from _start_timer() as a value of the
 >         identifier in the self._start_time which is dict-type"""
 > }}}
 > * In the following API doc, do you mean returns instead of raises?
 > {{{
 >         self._start_time after setting is successfully done. In case
 >         of stopping the timer which has never been started, it raises
 >         and does nothing. But in case of stopping the time which isn't
 > }}}
 Yes, but I've revised documentation for accuracy. Please read this.
 {{{
 927c2a9 [2225_statistics] simplify too detailed documentation in
 start_timer() and stop_timer()
 }}}

 > * I have pushed some comment updates to the `trac2225_statistics`
 branch. Please review them carefully and check if they are ok.

 I've read all. The changes are mainly based on English grammar, I think.
 These are very polite for other readers. Thanks a lot. I have to learn
 English more:(.

 I've also revised prefixes in the commit messages (`trac2225_` ->
 `2225_`). So the git IDs might be changed. Sorry for confusion.

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