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
Status: | assigned → needs_review |
---|
comment:2 Changed 11 months ago by
Status: | needs_review → needs_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
Reviewer: | → irl |
---|---|
Status: | needs_revision → needs_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 follow-up: 5 Changed 11 months ago by
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 follow-up: 6 Changed 11 months ago by
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 Changed 10 months ago by
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:8 Changed 10 months ago by
Status: | needs_review → merge_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
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
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
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!
Please review commit 421e413 in my task-29166 branch.