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-20130219
           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,

 I've already merged `trac2225_statistics_3`. Thank you for reviewing so
 quickly.

 BTW here is my replying to your comments to `trac2225_xfrout`.

 > * In `xfrin.py.in`, Can't the following code be simplified? The replace
 >   method calls assume that `DATAROOTDIR` has the word `share` in it
 >   anyway. BTW, does this original modification have any reason to be in
 >   the `trac2225_xfrout` branch?

 I think some users may change DATAROOTDIR to the directry other than
 `share` when they configures. So I left it. But I've changed the code for
 simplifying. Could you see this?
 {{{
 35c6e4a  2013-02-06  [2225_xfrout] setting SPECFILE_LOCATION is more
 simplified
 }}}
 Of course, this change isn't directly needed for current
 `trac2225_xfrout`. I needed in the old python statistics library.

 > * Is the same change (above) required in `xfrout.py.in` where
 >   `B10_FROM_SOURCE` doesn't seem to be used?

 Of course, it isn't required. But xfrout.spec is surely placed under
 `B10_FROM_SOURCE`. If some code other than `b10-xfrout` calls it from the
 build or source directly with the environment variable, it should be set
 properly. I don't now have strong opinion for it.

 > * In the code, as an example, `socket/unixdomain/senderr` is
 >   incremented. But the `b10-xfrout` manpage does not mention
 >   `socket/unixdomain/`.

 socket/unixdomain/senderr is mentioned here. Do you mean about how we
 should describe about it? Or should we change the name to
 `socket/unix/****`?

 b10-xfrout.xml:
 {{{#!xml
       <varlistentry>
         <term>senderr</term>
         <listitem><simpara>
          UNIX domain sockets send errors
         </simpara></listitem>
       </varlistentry>
 }}}

 > * In `UnixSockServer.__init__()`, what happens now if
 >   `ThreadingUnixStreamServer.__init__()` raises an exception?
 ...
 > Should it not raise the exception again after updating the counter?
 > There should be an `assertRaises` check in the corresponding testcase
 > too (in `test_open()`).

 I changed it to raise an exception again and to test for it. Could see
 this?
 {{{
 * 67b451a  2013-02-06  [2225_xfrout] raise again an exception after
 incrementing the counter
 }}}

 > * Here, why is `close` incremented inside the `try` block, when it's
 >   already shutdown outside the try block?

 Yes, that isn't needed. I've moved it out of the try block.
 {{{
 * 6ed2eb9  2013-02-06  [2225_xfrout] counting closed unixdomain sockets
 doesn't need to be inside the try block
 }}}

 > * A couple of `NotifyOut` tests seem to have been removed. Why?

 Of course, I removed some tests for `XfroutCouter` class long time ago(git
 c06c560). But the similar tests are done in other test classes. For
 example, regarding `NotifyOut` class, I think the tests for it are done
 under `isc/notify/notify_out/tests` as you know.

 > * Is there anything left to be done for this unittest:

 It isn't needed. I've removed at:
 {{{
 8bfa9b4  2013-02-06  [2225_xfrout] remove an empty unittest
 }}}

 > * I have not tested the lettuce changes as I have issues running lettuce
 >   on my box with Python 3.3 currently. I'll do this during the next
 >   round of review.

 OK, I'll wait for your second round.

 > * I've also pushed some minor patches. Please check them.

 That is reasonable for me. Thank you very much.

 Regards,

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