Opened 3 years ago

Closed 3 years ago

#20049 closed defect (fixed)

Adapt legacy module to accept bridge network statuses from two authorities

Reported by: karsten Owned by:
Priority: Very High Milestone:
Component: Metrics/Website Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There are currently two bridge authorities in the network, each of them seeing a distinct set of bridges. We should adapt network size statistics to add up set sizes for the daily number of bridges.

Child Tickets

Change History (5)

comment:1 Changed 3 years ago by karsten

Status: newneeds_review

Please review this branch.

comment:2 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

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.

comment:3 Changed 3 years ago by karsten

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?

comment:4 Changed 3 years ago by iwakeh

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

comment:5 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

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

Note: See TracTickets for help on using tickets.