Re: [tor-bugs] #16151 [metrics-lib]: Add new descriptor source to fetch descriptors from CollecTor via https

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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):

 Just mirroring the directory and handling the data later does seem to be
 the better approach.

 Thanks, for providing the ExoneraTor use-case!


 Here some comments and questions:

 1. In my opinion/experience it usually pays off very well to write tests
 immediately when introducing regex-matching. This would document precisely
 what type of directory is expected and help with furture additions and
 changes.

 2. As far a I can tell, you only want the "DescriptorCollector" to run
 once per instance? Why not just offer an interface consisting of one
 possibly overloaded method maybe with an additional boolean for the
 extraneous-file-cleanup (directories could be handled as List<String> or
 as String array)? If the method signature is too long, one could introduce
 a simple configuration class "CollectorConfiguration" as parameter for
 "collectRemoteFiles".

 3. Is the repeated call of "collectRemoteFiles" not a valid use case?

 4. Should the file-cleanup not be performed after each collector run?


 5. The return value of "parseDirectoryListing" is not used; and in case of
 a problem nothing is logged (or printed to System.err). Same with
 "fetchRemoteFile", here it might be even more valuable to know why the
 download failed.

 6. If the above suggestion (2.) is not used, the code might be a little
 more readable having the following method
 {{{
 private void ifRunningThrowIllegalState(String msg){
   if (this.hasStartedCollecting) {
     throw new IllegalStateException(msg);
   }
 }
 }}}
 replace all of the {{{if (this.hasStartedCollecting) ...}}} calls; it
 would also be easier to maintain.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16151#comment:7>
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