Opened 11 months ago

Closed 6 weeks ago

#21145 closed enhancement (fixed)

Adapt ExoneraTor to using metrics-base

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone:
Component: Metrics/ExoneraTor Version:
Severity: Normal Keywords: metrics-2017
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

see #20596

Child Tickets

Change History (16)

comment:1 in reply to:  description Changed 11 months ago by iwakeh

Replying to iwakeh:

see #20596

as far as feasible.

comment:2 Changed 3 months ago by karsten

Summary: adapt ExoneraTor to using metrics-baseAdapt ExoneraTor to using metrics-base

Capitalize summary.

comment:3 Changed 3 months ago by karsten

Keywords: metrics-2018 added

comment:4 Changed 3 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:5 Changed 2 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: newaccepted

comment:6 Changed 2 months ago by iwakeh

Status: acceptedneeds_review

Please review these changes, which are optimistically based on the patch for #19624 (two commits for checkstyle fixes and using commons-lang3).

comment:7 Changed 2 months ago by iwakeh

There is also an update to the latest metrics-lib 2.1.1 in the patch.

comment:8 Changed 2 months ago by karsten

Status: needs_reviewneeds_revision

Hmm, the patch looks good, but I'm having trouble executing the JAR file:

$ java -jar generated/dist/exonerator-1.0.0-dev.jar 
11:33:38.330 [main] ERROR org.torproject.exonerator.ExoneraTorDatabaseImporter - Could not connect to database.  Exiting.
java.sql.SQLException: No suitable driver found for jdbc:postgresql:exonerator
	at java.sql.DriverManager.getConnection(DriverManager.java:689)
	at java.sql.DriverManager.getConnection(DriverManager.java:270)
	at org.torproject.exonerator.ExoneraTorDatabaseImporter.openDatabaseConnection(ExoneraTorDatabaseImporter.java:102)
	at org.torproject.exonerator.ExoneraTorDatabaseImporter.main(ExoneraTorDatabaseImporter.java:51)

Any idea what could be missing?

(Minor nitpick: let's also remove the run-exonerator.sh file in this change. I can do that when merging the branch.)

comment:9 in reply to:  8 Changed 6 weeks ago by iwakeh

Status: needs_revisionneeds_review

Replying to karsten:

Hmm, the patch looks good, but I'm having trouble executing the JAR file:

[snip]

Any idea what could be missing?

Please find a commit in metrics-base and a fixup commit for the build.xml on top of my patch branch.
The jdbc jar and its service definition were missing.

(Minor nitpick: let's also remove the run-exonerator.sh file in this change. I can do that when merging the branch.)

No nitpick, I overlooked removing this file, which is part of a second fixup commit.

comment:10 Changed 6 weeks ago by karsten

Works! Pushed the metrics-base commit.

But before I squash and push the ExoneraTor commits, did you make the following change on purpose?

-    <include name="postgresql-9.4.1212.jar"/>
+    <include name="postgresql-jdbc3.jar"/>

I know that Debian stretch ships the JAR file as postgresql.jar together with three symbolic links:

lrwxrwxrwx 1 root root     14 Jan  9  2017 /usr/share/java/postgresql-9.4.1212.jar -> postgresql.jar
-rw-r--r-- 1 root root 650839 Jan  9  2017 /usr/share/java/postgresql.jar
lrwxrwxrwx 1 root root     14 Jan  9  2017 /usr/share/java/postgresql-jdbc3.jar -> postgresql.jar
lrwxrwxrwx 1 root root     14 Jan  9  2017 /usr/share/java/postgresql-jdbc4.jar -> postgresql.jar

But why pick the JDBC 3 link here rather than the one with the version number? That's different from what we do elsewhere.

If this change just slipped in, I'll undo it and merge the rest.

But if you intended to make this change, let's first discuss what names we should be using in build files in all metrics code bases.

comment:11 in reply to:  10 Changed 6 weeks ago by iwakeh

Replying to karsten:

Works! Pushed the metrics-base commit.

Thanks!

...
If this change just slipped in, I'll undo it and merge the rest.

Yes, please undo. I got tripped by those links and forgot to change it back.

comment:12 Changed 6 weeks ago by karsten

Hmm, I made that change and also updated to latest metrics-base (see my task-22145 branch, but now I'm having trouble rebasing to master. Do you mind doing the squashing and rebasing to master part?

comment:13 Changed 6 weeks ago by iwakeh

Any particular error? I'll squash&rebase.

comment:14 Changed 6 weeks ago by karsten

Just a merge conflict in build.xml that I couldn't resolve easily. You should be running into the same when trying to rebase. (Thanks!)

comment:15 Changed 6 weeks ago by iwakeh

Please review the rebased&squashed commit. While looking at build.xml I added another patternset to avoid redundant includes.

comment:16 Changed 6 weeks ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Amended by a metrics-base update and pushed to master. Closing. Thanks!

Note: See TracTickets for help on using tickets.