BIND 10 #917: update stats-httpd to return only specified statistics
BIND 10 Development
do-not-reply at isc.org
Tue Nov 8 08:28:00 UTC 2011
#917: update stats-httpd to return only specified statistics
-------------------------------------+-------------------------------------
Reporter: | Owner: UnAssigned
naokikambe | Status: reviewing
Type: | Milestone:
enhancement | Sprint-20111108
Priority: major | Resolution:
Component: | Sensitive: 0
statistics | Sub-Project: DNS
Keywords: | Estimated Difficulty: 6
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by naokikambe):
* owner: naokikambe => UnAssigned
Comment:
Hello,
Replying to [comment:10 vorner]:
> I fixed a small problem in the changelog text (and pushed), I hope you
don't mind.
Thank you very much.
> The test fail for me, maybe you'll know more about why this might
happen:
> {{{
> ======================================================================
> FAIL: test_do_GET (__main__.TestHttpHandler)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/vorner/work/bind10/src/bin/stats/tests/b10-stats-
httpd_test.py", line 349, in test_do_GET
> check_XSL_URL_PATH(mod=None, item=None)
> File "/home/vorner/work/bind10/src/bin/stats/tests/b10-stats-
httpd_test.py", line 300, in check_XSL_URL_PATH
> self.assertTrue(k in itm_vo)
> AssertionError: False is not true
> }}}
The path names of the XSL/XHTML structure in the !ElementTree obejct
were incorrect. The path name ended with the unnecessary slash. I
already fixed them at git f8a64959bc5f3ddf68ba4d01bee092bf4f1f9558. Please
see it.
> * In this code, it looks unclean to decide by the text of exception:
> {{{#!python
> # Couldn't find neither specified module name nor
> # specified item name
> if str(err).startswith('Stats module: specified arguments are
incorrect:'):
> self.send_error(404)
> else:
> self.send_error(500)
> }}}
> Would it be possible to throw inherited exception in that case?
I added a new exception !StatsHttpdDataError. It is raised when an
error due to the specified data is occurred. If it's raised,
stats-httpd will return 404 notfound. Please see git
df02b63fe1176c572a7eee996921f211ca970953.
> * This is quite obvious. However, it might be more helpful to note what
the parameters mean/do.
> {{{
> the data which obtains from it. args are owner and name."""
> }}}
> * The closures (`get_stats_data`, `get_stats_spec`, …) don't really
have a helpful docstring. It doesn't say what the functions actually do
and that the last parameter is where the result is stored.
I added the more docstrings. Please see git
8e8607c6faa34d9493a831054ecb64281f1f06c7 and git
4ab7d17edc10ce4f7b834709aa009aba4db9d877. If they are not enough, please
let me know.
> * The closures - it is not necessary to explicitly `return None`, if
there's no return, the function returns None by itself.
I removed such {{{return None}}}s. Please see git
50e96053742a30584f91a6bdb4b788977cd166bf.
> * Is it really OK to do this? Can't the conversion to us-ascii lose
some characters? Can't there be non-ascii characters in the XML?
> {{{
> xml_string = str(xml.etree.ElementTree.tostring(xml_elem,
encoding='utf-8'),
> encoding='us-ascii')
> }}}
Yes, it loses non-ascii characters. That was added in #1021 but that
might be a work around. However I think that such non-ascii characters
wouldn't be acceptable in the system. If we make that acceptable, we
have to totally review the specification of the whole system. I know
such non-ascii characters are urgent into the future, but do we need
it now?
> * The recursive closures detect where they are in the structure by the
type of parameter. Wouldn't it be better to write a separate function for
each of the cases? Because we should be sure what type there is when we
call it recursively.
Such internal functions are temporary in this ticket. I'd like to
this task would be bigger. Sorry, please give me any suggestions if
you have.
> * The test commented as `# 404 NotFound (nonexistent item name)` -
shouldn't it be tested on some module that exists? Because the module is
nonexistent too.
I added such testcases. Please see git
cc48074a9fec60ef9ba69991549f9e167e620225.
> * Shouldn't assertEqual be used here?
> {{{#!python
> if item['item_type'] == 'list':
> self.assertTrue(len(item) == 7)
> else:
> self.assertTrue(len(item) == 6)
> }}}
I fixed it. Please see git f1e08d75cabc45454a9bde86158dc8c7348d7f9d.
Best
--
Ticket URL: <http://bind10.isc.org/ticket/917#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list