Run modules from Java only

[karsten]

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.

[karsten]

Please review ​commit 421e413 in my task-29166 branch.

[karsten]

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

[karsten]

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!

[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.

[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!

[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.

[irl]

Planned for review party tomorrow.

[irl]

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).

[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.

[karsten]

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!