[tor-bugs] #21759 [Metrics/CollecTor]: Add persistence for torperf/onionperf

[Tor Bug Tracker & Wiki]

#21759: Add persistence for torperf/onionperf
-------------------------------+---------------------------------
 Reporter:  iwakeh             |          Owner:  iwakeh
     Type:  enhancement        |         Status:  needs_revision
 Priority:  Medium             |      Milestone:  CollecTor 1.3.0
Component:  Metrics/CollecTor  |        Version:
 Severity:  Normal             |     Resolution:
 Keywords:                     |  Actual Points:
Parent ID:                     |         Points:
 Reviewer:                     |        Sponsor:
-------------------------------+---------------------------------
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 It took a bit longer to respond to your comment 5, but here it comes:
  - The following is entirely unrelated to the given patch, but can you try
 to change your Git commit messages to follow the 50/72 rule? That is, the
 first line has no more than 50 characters (unless that's just too hard, in
 which case 60 are still okay), the next line is blank, and all remaining
 lines are wrapped at 72 characters with newlines added between paragraphs.
 And can you write the first line using the imperative, as in "Add sync
 functionality for tpf files." rather than "Implements XY"? Again, this is
 unrelated to this specific commit, but just like we're trying to use a
 common code style we should aim for using a common commit message style.
 Sorry for the nitpick. :)
  - Can you add a change log entry for this change?
  - Should we put out a metrics-lib 2.1.0 first towards the end of the
 month, so that we don't have to require a -dev version in the next
 released CollecTor?
  - Can we keep the old capitalization of OnionPerf in the configs? It
 seems that's what Rob used. It also means fewer changes to the code.
  - Typo: "Instantiate", not "Instanciate".
  - The line `byte[] db = desc.getRawDescriptorBytes();` in
 `SyncPersistence` is probably still left over from testing. This method
 has become really expensive, so we should be careful not to call it more
 often than necessary.
  - I'd like to avoid that we handle descriptors differently depending on
 whether we download them from an OnionPerf host vs. synchronize them from
 a CollecTor host. And I believe that the approach taken in this
 synchronization patch makes more sense than the strict validation approach
 taken in the download part. But maybe we can first take a step back and
 look at all the other modules and find a common approach for all or at
 least most of them. Let's have this discussion first and then merge this
 patch if it still does what we think should be done. Does that make sense?

 Thanks!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21759#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online