Opened 10 months ago

Closed 9 months ago

#28859 closed enhancement (fixed)

Use Java 8 date-time functionality in ExoneraTor

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

Description

We had plans to use Java 8 date-time functionality in all our code bases for quite a while now. I finally picked this up by reading a few tutorials, mostly to figure out which of the three types we're going to use: Instant, LocalDateTime, or ZonedDateTime. I picked ExoneraTor for this discussion/review, because nothing else depends on it and we're currently not working on this code base in other tickets.

So, I think I have a plan now, and I'm presenting it here using snippets from my ExoneraTor patch:

@@ -131,10 +132,11 @@ public class ExoneraTorDatabaseImporter {
     try {
       if (lockFile.exists()) {
         BufferedReader br = new BufferedReader(new FileReader(lockFile));
-        long runStarted = Long.parseLong(br.readLine());
+        Instant runStarted = Instant.ofEpochMilli(Long.parseLong(
+            br.readLine()));
         br.close();
-        if (System.currentTimeMillis() - runStarted
-            < 6L * 60L * 60L * 1000L) {
+        if (runStarted.plus(Duration.ofHours(6L))
+            .compareTo(Instant.now()) >= 0) {
           logger.warn("File 'exonerator-lock' is less than 6 "
               + "hours old.  Exiting.");
           System.exit(1);

This code is used to write the current system time to a local lock file to determine when we need to obey that or can safely delete it. Let's ignore the design and just look at the date-time part for now.

So, I think that we can replace code where we used System.currentTimeMillis() in the past with Instant. The concept is close enough to the "machine time" concept rather than "human time" that Instant will work for us. We care about hours here, not things like calendar months.

@@ -223,7 +225,8 @@ public class ExoneraTorDatabaseImporter {
 
   /* Parse a consensus. */
   private static void parseConsensus(RelayNetworkStatusConsensus consensus) {
-    long validAfterMillis = consensus.getValidAfterMillis();
+    LocalDateTime validAfter = LocalDateTime.ofInstant(Instant.ofEpochMilli(
+        consensus.getValidAfterMillis()), ZoneOffset.UTC);
     for (NetworkStatusEntry entry : consensus.getStatusEntries().values()) {
       if (entry.getFlags().contains("Running")) {
         String fingerprintBase64 = null;

Things are a bit different here. The valid-after time in a consensus is not just a point on the timeline. It's an actual date and time that has a meaning for humans. That somewhat rules out the Instant.

Now, why use LocalDateTime here und not ZonedDateTime, as suggested before? Well, I could imagine doing either, as long as we're consistent across our codebases.

However, one reason that made me pick LocalDateTime in the end was the comparison with PostgreSQL's TIMESTAMP [WITHOUT TIMEZONE] vs. TIMESTAMP WITH TIMEZONE types. We're importing lots of timestamps into PostgreSQL, but we're always using TIMESTAMP [WITHOUT TIMEZONE]. Because we know that timestamps in Tor are always UTC and we simply don't need timezone information. The corresponding types for TIMESTAMP [WITHOUT TIMEZONE] and TIMESTAMP WITH TIMEZONE are LocalDateTime and OffsetDateTime, by the way.

The only use case I see where LocalDateTime could be problematic is when somebody wants to convert our timestamps to their local timezone. But that's not a common use case for us, and we're primarily building our tools for ourselves and the services we provide.

@@ -248,26 +251,21 @@ public class ExoneraTorDatabaseImporter {
[...]
     try {
       for (String orAddress : orAddresses) {
         insertStatusentryStatement.clearParameters();
-        insertStatusentryStatement.setTimestamp(1,
-            new Timestamp(validAfterMillis), calendarUTC);
+        insertStatusentryStatement.setObject(1, validAfter);
         insertStatusentryStatement.setString(2, fingerprintBase64);
         if (!orAddress.contains(":")) {
           insertStatusentryStatement.setString(3, orAddress);

This is what it looks like when we're passing a LocalDateTime to the database.

@@ -47,7 +50,9 @@ public class QueryServlet extends HttpServlet {
 
   private DataSource ds;
 
[...]
+  private static final DateTimeFormatter validAfterTimeFormatter
+      = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")
+      .withZone(ZoneOffset.UTC);
 
   @Override
   public void init() {

And this is how we're parsing/formatting a Tor date-time. It's not a standard format, but we can easily define it once per class or package or codebase. Yay, thread safety!

Okay, these are the highlights. I'll post the branch once I have a ticket number.

Child Tickets

Change History (4)

comment:1 Changed 10 months ago by karsten

Status: assignedneeds_review

comment:2 Changed 10 months ago by irl

Reviewer: irl

comment:3 Changed 10 months ago by irl

Status: needs_reviewmerge_ready

Ok. I think this is somewhat like UTF-8 encoding/decoding in that we should be explicit whenever times come into and leave the application that they are converted to "local" time where this is the local time for the application, and for our use this will always be UTC. Let's just watch out for this.

The patch looks good to me.

comment:4 in reply to:  3 Changed 9 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Replying to irl:

Ok. I think this is somewhat like UTF-8 encoding/decoding in that we should be explicit whenever times come into and leave the application that they are converted to "local" time where this is the local time for the application, and for our use this will always be UTC. Let's just watch out for this.

Makes sense. It's something to document clearly.

The patch looks good to me.

Great, thanks for looking! Merged. Closing.

Note: See TracTickets for help on using tickets.