Adapt legacy module to accept bridge network statuses from two authorities

added component::metrics/website priority::very high resolution::fixed severity::normal status::closed type::defect labels

Please review this branch.

Trac:
Status: new to needs_review

Some findings below; this is difficult to review b/c of the ancient code base. I assume it does what is intended.

build.xml still references descriptor-1.2.0
It could be useful to consider enums for authority listings and EnumMap, as these might be faster than hash-maps, but ok for the moment.
In the existing code bw.append(line + "\n"); should be replaced by the two statements bw.append(line); bw.newLine();.
I would place a comment differently and leave out the empty else. Minor change suggestion w/o enum change:

diff --git a/modules/legacy/src/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java b/modules/legacy/src/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java
index 631839d..8d51d5d 100644
--- a/modules/legacy/src/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java
+++ b/modules/legacy/src/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java
@@ -139,6 +139,8 @@ public class ConsensusStatsFileHandler {
                 + "! Aborting to read this file!");
             break;
           }
+          /* Assume that all lines without authority nickname are based on
+           * Tonga's network status, not Bifroest's. */
           String key = parts[0] + "," + (parts.length < 4 ? "Tonga" : parts[1]);
           String value = null;
           if (parts.length == 2) {
@@ -147,11 +149,8 @@ public class ConsensusStatsFileHandler {
             value = key + "," + parts[1] + "," + parts[2];
           } else if (parts.length == 4) {
             value = key + "," + parts[2] + "," + parts[3];
-          } else {
-            /* Impossible, we already checked the range above. */
-          }
-          /* Assume that all lines without authority nickname are based on
-           * Tonga's network status, not Bifroest's. */
+          } /* No more cases as we already checked the range above. */
+
           this.bridgesRaw.put(key, value);
         }
         br.close();
@@ -308,7 +307,8 @@ public class ConsensusStatsFileHandler {
           new FileWriter(this.bridgeConsensusStatsRawFile));
       bw.append("datetime,authority,brunning,brunningec2\n");
       for (String line : this.bridgesRaw.values()) {
-        bw.append(line + "\n");
+        bw.append(line);
+        bw.newLine();
       }
       bw.close();
       this.logger.fine("Finished writing file "
@@ -404,7 +404,7 @@ public class ConsensusStatsFileHandler {
               + "old: " + this.bridgesRaw.lastKey());
         }
       } catch (ParseException e) {
-         /* Can't parse the timestamp? Whatever. */
+        logger.warning("Can't parse the timestamp? Reason: " + e);
       }
     }
     logger.info(dumpStats.toString());

This module should be refactored very soon! But, that's a different ticket.

Should there also be a follow-up ticket for accommodating future authority changes/additions more elegantly?

Ready for merge as hot-fix.

Trac:
Status: needs_review to merge_ready

Thanks for the quick review! I made the changes you suggested and pushed to master. And I totally agree that this module should be refactored soon. I started doing that a while ago, but as most things, it quickly turned into a mid-size project that I wasn't able to complete yet. I suggest we close this ticket and create a new one for rewriting this module as soon as we get to that. Okay?

I opened #20059 (moved) as a planning ticket. This can be closed when all is merged.

I think you mean #20053 (moved), not #20059 (moved). Looks good, thanks! Closing.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #20053 (moved)

Adapt legacy module to accept bridge network statuses from two authorities

Child items 0

Activity