Opened 11 months ago

Closed 9 months ago

#29166 closed enhancement (fixed)

Run modules from Java only

Reported by: karsten Owned by: karsten
Priority: Medium Milestone:
Component: Metrics/Statistics Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: irl Sponsor:

Description

As discussed earlier, I'd like to run all modules for the daily updater from Java rather than using Ant. This is the first step in streamlining things and re-using code between modules. Posting a branch in a second.

Child Tickets

Change History (10)

comment:1 Changed 11 months ago by karsten

Status: assignedneeds_review

comment:2 Changed 11 months ago by karsten

Status: needs_reviewneeds_revision

Taking this out of review until I have resolved all the path issues that I just discovered.

comment:3 Changed 11 months ago by karsten

Reviewer: irl
Status: needs_revisionneeds_review

There, revised my earlier patch and assigned irl as reviewer as discussed today. Please review that commit above plus commit d8049ba in the same branch. Thanks!

comment:4 Changed 11 months ago by 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.


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.


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.

comment:5 in reply to:  4 ; Changed 11 months ago by karsten

Replying to 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!

comment:6 in reply to:  5 Changed 10 months ago by karsten

Replying to karsten:

Replying to notirl:

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.

We briefly talked about this in Brussels and decided that it makes more sense for (not)irl to do another review and some local tests after Brussels.

comment:7 Changed 10 months ago by irl

Planned for review party tomorrow.

comment:8 Changed 10 months ago by irl

Status: needs_reviewmerge_ready

Ok, checks and tests pass, I've not actually run the code but I did look over it before Brussels in more detail and it looked good then. I don't see any reason to delay this deployment (except don't do it 5pm Friday because weekend).

comment:9 Changed 9 months ago by karsten

Cool! I ran this using the integration tests from #29425 and spotted a bug. Fixed that bug and pushed to master:

diff --git a/src/main/java/org/torproject/metrics/stats/connbidirect/Main.java b/src/main/java/org/torproject/metrics/stats/connbidirect/Main.java
index f4743af9..c7fefee7 100644
--- a/src/main/java/org/torproject/metrics/stats/connbidirect/Main.java
+++ b/src/main/java/org/torproject/metrics/stats/connbidirect/Main.java
@@ -130,7 +130,7 @@ public class Main {
   static final long ONE_DAY_IN_MILLIS = 86400000L;
 
   private static final File baseDir = new File(
-      org.torproject.metrics.stats.main.Main.modulesDir, "advbwdist");
+      org.torproject.metrics.stats.main.Main.modulesDir, "connbidirect");
 
   /** Executes this data-processing module. */
   public static void main(String[] args) throws IOException {

Will deploy later today and then close.

comment:10 Changed 9 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

This is deployed as of Thursday/Friday, and the weekend runs looked normal. Closing. Yay, this is a big step towards making metrics-lib more maintainable in the future!

Note: See TracTickets for help on using tickets.