[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #21759 [Metrics/CollecTor]: Add persistence for torperf/onionperf
#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:
-------------------------------+---------------------------------
Comment (by iwakeh):
Replying to [comment:8 karsten]:
> It took a bit longer to respond to your comment 5, but here it comes:
Thanks for reviewing!
> - 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. :)
No nitpicking; we even have that
[https://trac.torproject.org/projects/tor/wiki/org/teams/MetricsTeam/ContributorGuide#PatchFormat
documented]. Will adapt the commits.
> - Can you add a change log entry for this change?
Sure.
> - 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?
Agreed, the new CollecTor release should only use release versions of
metrics-lib.
Wouldn't the next be 2.0.1 for the tiny fix? Are there more things we
could release soon?
Maybe continue the metrics-lib release discussion on #22912 or via mail as
it feels out of place here?
> - 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.
Well, in general I'd prefer:
1. consistent naming and capitalization of classes and configuration
(i.e., OnionperfDownloader.class might need to be renamed too, if
'OnionPerf*' is chosen.)
2. rather less capitalization, e.g., prefer 'Exitlist' over 'ExitList'
Maybe, there are more rules for naming.
> - 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.
Get to these two in a new patch when the final topic is addressed.
> - 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.
Yes, I agree the simpler version will be better.
> 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?
Totally, I'll survey that question as next step before any other work
here.
> Thanks!
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21759#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs