BIND 10 #2252: Implement counters into Xfrin (1/3)
BIND 10 Development
do-not-reply at isc.org
Wed Mar 13 08:41:29 UTC 2013
#2252: Implement counters into Xfrin (1/3)
-------------------------------------+-------------------------------------
Reporter: naokikambe | Owner:
Type: enhancement | jinmei
Priority: medium | Status:
Component: xfrin | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130319
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
-------------------------------------+-------------------------------------
Changes (by naokikambe):
* owner: naokikambe => jinmei
Comment:
Hello, thank you for reviewing!
Replying to [comment:11 jinmei]:
> '''general'''
>
> - Don't we need a changelog entry?
Of course, we need. Please see [comment:2 this].
> - after struggling with figuring out how `XfrinConnection` statistics
> work I realized it relied on a global (singleton) statistics object
> for the module. I don't know what kind of prior discussion brought
> that decision, but I still think that's a bad practice. Singletons
> make the code more untestable and more fragile (as it often
> builds hidden dependency between separated modules; when one module
> modifies a singleton it implicitly affects the other), trading its
> superficial convenience. IMO it should be avoided unless there's
> something inherently impossible without using singletone. Is there
> any such strong reason in this case?
Sorry, but I can't understand well whether there are such harmful things
from the current code. However, regarding tests of the statistics library,
they work well so far on the build farm as long as I see. Can we consider
about that if some problem occurs which you worry about in the future?
> '''b10-xfrin.xml'''
>
> - This looks awkward to me in various points:
> {{{
> <term>time_to_ixfr</term>
> <listitem><simpara>
> Elapsed time in second to do the last IXFR
> </simpara></listitem>
> }}}
> First off, I don't understand the description. Second, with my
> understanding what this counter is from the code, "time_to_ixfr"
> doesn't seem to represent what it means. My understanding of this
> counter is "Elapsed time since the start of currently running IXFR,
> or, if it's not currently running, the duration of the previous
> IXFR" (correct?). If so, I'd rename it, e.g.,
> "latest_ixfr_duration". Same for AXFR. And also update the
> description.
The latter one is correct. I revised the names as you pointed out. Could
you see the following commit?
{{{
75445a1 [2252] rename the item name and update descriptions
}}}
> - as for time_to_ixfr, what if there's never been an xfr for the zone?
It shows still the default value, 0.0.
>
> '''xfrin.py.in'''
>
> - get_ipver_str: I suggest making it "protected" by renaming
> `_get_ipver_str`. I'd also clarify it's mostly private except for
> tests (which is my understanding).
I made it protected as you mentioned. Could you see the following commit?
{{{
a1b3cfd [2252] rename the helper method name
}}}
> - get_ipver_str: what if family is neither INET6 nor INET?
I considered the case where `None` value is returned. Could you see the
following commit?
{{{
4de51f8 [2252] care for the socket family which is neither INET6 nor INET
}}}
> - `_check_soa_serial`: shouldn't this be distinguished per name and
> class? both class IN and CH can have "example.com", for example.
> {{{#!python
> # count soaoutv4 or soaoutv6 requests
> self._counters.inc('zones', self._zone_name.to_text(),
> 'soaout' + self.get_ipver_str())
> }}}
> This comment applies to all other counters and timers.
Probably should be considered. As an example, we might need to introduce a
`classes` layer over the current `zones` layer in the statistics data
structure. But this is too big to handle in this ticket. Could we consider
in other ticket?
>
> - do_xfrin: not directly related to this branch, but using this
> opportunity I suggest simplifying this part:
> {{{#!python
> # Right now RRType.[IA]XFR().to_text() is 'TYPExxx', so we
need
> # to hardcode here.
> req_str = 'IXFR' if request_type == RRType.IXFR else 'AXFR'
> }}}
> as follows:
> {{{#!python
> req_str = request_type.to_text()
> }}}
> we should now be able to do that.
I applied it. Could you see the following commit?
{{{
01b2a2f [2252] simplify complicated if-else lines (unrelated to #2252)
}}}
> - do_xfrin: stop_timer wouldn't be called if exception is raised
> during the process.
I think it is okay that the timer count would be discarded if exception is
raised.
> - do_xfrin: maybe a matter of taste, but the `final` clause could be
> simplified:
> {{{#!python
> counter_dict = {XFRIN_OK: 'xfrsuccess', XFRIN_FAIL,
'xfrfail'}
> self._counters.inc('zones', self._zone_name.to_text(),
> counter_dict[ret])
> }}}
I revised the code as you proposed. Could you see the following commit?
{{{
2cfdda6 [2252] simplify complicated if-elif lines using a dictionary
}}}
> - command_handler: why do we bother to log it here?
> {{{#!python
> logger.debug(DBG_XFRIN_TRACE,
XFRIN_RECEIVED_GETSTATS_COMMAND,
> str(answer))
> }}}
> I'm not necessarily opposed to logging this event per se, but it's
> awkward to see only this command is logged especially if it's at a
> debug level.
The message is used in the lettuce tests. but it's too annoying, so I
removed `{ poll-interval: 1}` from stats config in lettuce. Could you see
the following commit?
{{{
4d94e85 [2252] remove poll-interval from stats config
}}}
>
> '''xfrin_test.py'''
>
> - in general, I see many similar patters repeated like this:
> {{{#!python
> self.assertEqual(1, self.conn._counters.get('zones',
TEST_ZONE_NAME_STR,
> 'soaoutv4'))
> }}}
> I'd like to unify them.
I unified them. Could you see the following commit?
{{{
936b3ff [2252] unify redundant assertion lines
}}}
> - test_ipver_str: do we have to reset `self.conn.socket`? I guess
> it's re-constructed for every test case.
I revised the code to reset the socket object within the test case. Could
you see the following commit?
{{{
0ef3b7a [2252] test _get_ipver_str() by creating MockXfrinConnection
object
}}}
> - test_soacheck doesn't confirm soaoutv6 is incremented.
I added test cases for IPv6. Could you see the following commit?
{{{
7fe200c [2252] add tests for counters of XfrinConnection in IPv6 address
}}}
> - test_do_xfrin: likewise. it's redundant, and yet incomplete.
I included in the above commits.
> - isn't this mostly trivial?
> {{{#!python
> self.assertGreaterEqual(self.conn._counters.get('zones',
>
TEST_ZONE_NAME_STR,
> 'time_to_axfr'),
> 0.0)
> }}}
> (the only other case is failure with exception and a negative value).
> This seems to suggest we should revisit how we test these in the
> first place. Checking the resulting counter values via get() is
> indirect and less reliable. It's probably better to steal inc() etc
> and confirm it's called at the expected timing on expected counter(s).
> It's not only for this case, but in general.
Thank you for implying. but regarding this part, I changed it to a bit
more accurate check by using `assertAlmostEqual`. Could you see the
following commit?
{{{
fcf8b9d [2252] replace assertGreaterEqual with assertAlmostEqual
}}}
>
> '''b10-xfrin.xml'''
>
> I made some suggested editorial changes directly. I also suggest
> updating time_to_axfr and time_to_ixfr (see above). Same for
> xfrin.spec.
Thank you for revising. I also updated the man page. The changes were
included in the above commit.
>
> '''lettuce tests'''
> - repeating this pattern is noisy and less efficient (we need to
> issue bindctl command for each line:
> {{{
> Then the statistics counter notifyoutv4 for the zone _SERVER_ should
be 0
> Then the statistics counter notifyoutv6 for the zone _SERVER_ should
be 0
> ...
> }}}
> I suggest unifying them in a single bindctl command as much as
> possible. see queries.feature.
I consolidated such same repeated patterns by following the way of
`queries.feature`. I update the step function to adopt this. Could you see
the following commits?
{{{
85aef71 [2252] add evaluating a minimum or maxmimum value
55a49c6 [2252] unify repeating similar patterns
aca3456 [2252] remove an obsoleted statistics step method
}}}
> - also, overall, (it appears) only a subset of counters are actually
> checked to be incremented. is it okay?
I added some tests for IPv4. Could you see the following commits?
{{{
be67ff2 [2252] add a test scenario for handling incoming notify only in
IPv4
d6d0432 [2252] add a test scenario for Xfr request rejected in IPv4
}}}
> - can we always assume this?
> {{{
> Then the statistics counter time_to_axfr for the zone _SERVER_
should be greater than 0.0
> Then the statistics counter time_to_axfr for the zone example.org.
should be greater than 0.0
> }}}
> What if xfr is completed super fast?
I considered the case that the transferring would be completed in 0.0
second. This change was included in the above commits.
> - should we repeat the initial statistics again?
> {{{
> #
> # Test6 for Xfrin statistics
> #
> # check initial statistics
> #
> }}}
> If so, I'd consider unifying them somehow.
I unified them. Could you see the following commit?
{{{
d69fb64 [2252] unify multiple similar checks of initial statistics into a
step method
}}}
> - why "greater than 0"? can't we be more specific?
> {{{
> Then the statistics counter soaoutv6 for the zone _SERVER_ should be
greater than 0
> Then the statistics counter soaoutv6 for the zone example.org.
should be greater than 0
> }}}
I reconsidered such expected values for ranges which the counter values
can be. Could you see the following commit?
{{{
655179c [2252] reconsider expected values of counters for more specific
values
}}}
> - is this correct?
> {{{#!diff
> - # Test1 for Xfrout statistics
> + # Test9 for Xfrout statistics
> }}}
> This is in a new scenario.
I reset the numbers of tests. Could you see the following commit?
{{{
cb5eb7d [2252] reset numbers in the scenario tests
}}}
Regards,
--
Ticket URL: <http://bind10.isc.org/ticket/2252#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list