[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #22836 [Metrics/Website]: Parse CollecTor's index.json and provide our own directory listing
#22836: Parse CollecTor's index.json and provide our own directory listing
-----------------------------+------------------------------
Reporter: karsten | Owner: karsten
Type: enhancement | Status: needs_review
Priority: Medium | Milestone:
Component: Metrics/Website | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
-----------------------------+------------------------------
Comment (by karsten):
Replying to [comment:17 iwakeh]:
> I cannot find a test for `static List<String[]> formatTableEntries`, did
you forget a commit?
I did not include one, because I didn't find an easy way to test this
method. But I like your new test method and think it already covers this
method quite well. Do you think we need to test more?
> The waiting time shouldn't be extended unnecessarily:
> {{{
> - } while (System.currentTimeMillis() < waitingSinceMillis + 10000L);
> + } while (null == this.index
> + && System.currentTimeMillis() < waitingSinceMillis + 10000L);
> }}}
Ouch. Yes, I meant to write that. Good catch!
> In addition, it might be safer and shorter to simply use
`AtomicReference` instead of the synchronized methods.
Works for me.
> Actually, I was hinting for a separate DirectoryListing class (in
comment:14), but separating the retrieval of the index is important, too.
> The separation you provided made it easy to introduce a
`CollectorDirectoryProvider`, which fetches and provides the index incl.
the suggested use of `AtomicReference` and the second while-condition.
DirectoryListing is a `Map<String, List<String[]>>` and encapsulates the
conversion from Index* to lists of strings. `CollectorDirectoryProvider`
only manages the scheduler and handing out the index.
Looks good to me!
> Another test `testListing` needs close inspection; it uses a fantasy
index.json and checks the resulting map.
Looks good!
> Please, review the commit on top of
[https://gitweb.torproject.org/user/iwakeh/metrics-
web.git/commit/?h=task-22836 this branch].
>
> (I didn't change the byte-calculations, b/c they are only 'decoration'
and the compiler will make sure that a constant is created for the
Math.log(1024).)
I only made a few trivial tweaks in [https://gitweb.torproject.org/karsten
/metrics-web.git/log/?h=task-22836 my updated task-22836 branch] that is
based on yours. If you like them, do you mind me squashing all commits
into one before rebasing and pushing to master?
Thanks for the review and improvements!
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22836#comment:18>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs