[tor-bugs] #22836 [Metrics/Website]: Parse CollecTor's index.json and provide our own directory listing

Tor Bug Tracker & Wiki blackhole at torproject.org
Mon Sep 18 19:37:20 UTC 2017


#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


More information about the tor-bugs mailing list