[tor-bugs] #16151 [metrics-lib]: Add new descriptor source to fetch descriptors from CollecTor via https
Tor Bug Tracker & Wiki
blackhole at torproject.org
Sat May 23 15:15:42 UTC 2015
#16151: Add new descriptor source to fetch descriptors from CollecTor via https
-----------------------------+--------------------------
Reporter: karsten | Owner: karsten
Type: enhancement | Status: needs_review
Priority: normal | Milestone:
Component: metrics-lib | Version:
Resolution: | Keywords:
Actual Points: | Parent ID:
Points: |
-----------------------------+--------------------------
Comment (by iwakeh):
Is there a task already for switching to java 7 and adding some logging
framework?
These two comprise my "ceterum censeo ..." :-)
In general, could this change be also used to softly introduce some
logging into metrics-lib?
Now the code comments/questions:
(I hope I didn't miss any new test cases ...)
It might be helpful to add some tests for DescriptorCollectorImpl that
explicit test for certain behavior.
These would be a very good source of documentation: e.g. take the comment
inside "putTogetherHistory". A test could explicitly verify the
functionality
described in the comments.
DescriptorCollector interface:
Why not turn the comments into javadoc? They are there already and very
readable. Just adding the tags for return values and parameters is not a
big deal.
Concerning DescriptorCollectorImpl:
Let's try to avoid the TODOs here from the beginning ;-)
For the various constants (COLLECTOR_BASE_URL, CONNECT_TIMEOUT_MILLIS,
READ_TIMEOUT_MILLIS)
I would suggest using a system property (defaulting to the current values)
as for the class
names in DescriptorSourceFactory using
{{{
onionoo.collector.url
onionoo.collector.timeout.read
onionoo.collector.timeout.write
}}}
as property names. Then it will be configurable immediately.
The retrieval of the 'int' properties needs just one extra method that
returns
either the converted property value or logs an error and returns the
default.
The URL could also be verified, if there is time to implement it now.
The other TODO is the Apache's directory list parsing: the regex parsing
approach is straigthforward and ok, but I'd make it testable and definitly
log any problems. So a method 'addDirsFromListing(SortedMap<String, Long>
list)'
with the code from lines 177-200 and the additional logging might be
useful.
Well, and the test for parsing directory string needs to be added.
Tests for the property handling above would be helpful, too.
What about adding the {{{@Override}}} annotation where appropriate?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16151#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list