[metrics-bugs] #29166 [Metrics/Statistics]: Run modules from Java only

[Tor Bug Tracker & Wiki]

#29166: Run modules from Java only
--------------------------------+------------------------------
 Reporter:  karsten             |          Owner:  karsten
     Type:  enhancement         |         Status:  needs_review
 Priority:  Medium              |      Milestone:
Component:  Metrics/Statistics  |        Version:
 Severity:  Normal              |     Resolution:
 Keywords:                      |  Actual Points:
Parent ID:                      |         Points:
 Reviewer:  irl                 |        Sponsor:
--------------------------------+------------------------------

Comment (by karsten):

 Replying to [comment:4 notirl]:
 > {{{
 > this.connection = DriverManager.getConnection(jdbcString);
 > }}}
 >
 > Should this not still be explicitly `this.jdbcString`? It's still a
 class member and not a local variable.

 It's indeed a class variable now, not an instance variable anymore, so we
 cannot use `this.` anymore. Of course, we could argue whether an instance
 variable might still make more sense. I didn't pay too much attention to
 this, because I'm planning to simplify code around database access across
 modules very soon.

 > Some of the paths have been defined with `/` in them, which breaks
 compatibility with non-Unix operating systems. (Windows uses `\` and RISC
 OS uses `:` as examples.) I think we don't care but it's worth being aware
 of it.
 >
 > We should have a policy and be explicit about our use of, or avoidance
 of, java.nio.file.Paths.

 I agree that using `Paths` would be cleaner. I tried to keep `Paths` but
 then realized that it's producing fewer code changes to just switch (back)
 to `File` in some places. Again, this is code where I anticipate major
 changes in the near future. For example, I'd like to have a single place
 in the code that reads descriptors and passes them to the modules, much
 like how Onionoo works. That's going to eliminate a lot of places where we
 had previously used `Paths`.

 > In general this looks OK, probably is best to not deploy it just before
 Brussels though. I can give it a more thorough review after Brussels.

 I think that not deploying before Brussels is a fine plan. Maybe we can
 talk about the review ''in'' Brussels and also do the deployment together.

 Thanks for this initial review!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/29166#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online