BIND 10 #2252: Implement counters into Xfrin (1/3)
BIND 10 Development
do-not-reply at isc.org
Fri Mar 8 05:00:43 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
-------------------------------------+-------------------------------------
Comment (by jinmei):
'''general'''
- Don't we need a changelog entry?
- 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?
'''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.
- as for time_to_ixfr, what if there's never been an xfr for the zone?
'''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).
- get_ipver_str: what if family 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.
- 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.
- do_xfrin: stop_timer wouldn't be called if exception is raised
during the process.
- 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])
}}}
- 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.
'''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.
- test_ipver_str: do we have to reset `self.conn.socket`? I guess
it's re-constructed for every test case.
- test_soacheck doesn't confirm soaoutv6 is incremented.
- test_do_xfrin: likewise. it's redundant, and yet incomplete.
- 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.
'''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.
'''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.
- also, overall, (it appears) only a subset of counters are actually
checked to be incremented. is it okay?
- 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?
- should we repeat the initial statistics again?
{{{
#
# Test6 for Xfrin statistics
#
# check initial statistics
#
}}}
If so, I'd consider unifying them somehow.
- 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
}}}
- is this correct?
{{{#!diff
- # Test1 for Xfrout statistics
+ # Test9 for Xfrout statistics
}}}
This is in a new scenario.
--
Ticket URL: <http://bind10.isc.org/ticket/2252#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list