[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #21272 [Metrics]: Onionperf deployment
#21272: Onionperf deployment
-------------------------+------------------------------
Reporter: hiro | Owner: metrics-team
Type: enhancement | Status: needs_review
Priority: Medium | Milestone:
Component: Metrics | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------+------------------------------
Comment (by iwakeh):
Replying to [comment:26 karsten]:
> Replying to [comment:25 iwakeh]:
> > Replying to [comment:23 karsten] and what's left from [comment:18]:
> > > iwakeh, please find my updated task-21272 branch with some tweaks as
discussed above.
> >
> > Thanks, for these changes!
> >
> > Couldn't `downloadFromOnionPerfHost` do some of the filename checking
before
> > calling `downloadAndParseOnionPerfTpfFile`?
>
> Well, that wouldn't change functionality but would be a simple
refactoring, right? What's the goal there? Make methods more testable or
easier to read or something else? In any case, would you want to suggest
new methods, and I'll move around code? Or do you want to work on a
patch?
No, I intended to avoid the [https://gitweb.torproject.org/karsten
/metrics-
db.git/tree/src/main/java/org/torproject/collector/torperf/TorperfDownloader.java?h=task-21272&id=3318eb8ca769392cca1a6ddc3344c43eba62da91#n750
superfluous parsing] of the URL for each file and avoid download when the
filename doesn't make sense.
No refactoring.
>
> > About the older code:
> > Could we just also make the change for #20514 now?
> > In addition, there are quite a few places where try-with resources
> > or some of `Files`' methods would prevent unclosed readers/writers.
> > Still this older code needs even more work, sigh. Maybe, after
Amsterdam?
>
> No need to spend time on this. Let's just remove the Torperf code in a
few weeks when we're certain that the OnionPerfs can take over.
That's fine, too.
>
> > And, regarding the descriptor parsing don't the checks inside
[https://gitweb.torproject.org/karsten/metrics-
db.git/tree/src/main/java/org/torproject/collector/torperf/TorperfDownloader.java?h=task-21272&id=3318eb8ca769392cca1a6ddc3344c43eba62da91#n797
this loop] belong into descriptor/metrics-lib itself?
> > (that might be a new ticket, though)
>
> Well, if we moved that code to metrics-lib, users wouldn't be able to
read Torperf results from anything else than the originally named .tpf
file. We usually avoid dependencies on file names if we can. This case
is a bit different, because we're archiving .tpf files, and we should be
certain that they contain what they say. I'd say that's specific to the
CollecTor case though and cannot be generalized in metrics-lib.
There could be a boolean parameter for strict checking?
But, I didn't mean to hijack this ticket. I can just write that down on a
list on my desk ;-)
>
> Note that we could have picked a different approach by parsing
descriptors from files and appending them to new files with file names
taken from descriptor contents. The result might be the exact same
output, or it could be a file with fewer descriptors or descriptors in a
different order, etc. But I felt it's easier to verify .tpf file contents
and either take them or leave them.
Hmm, ok, good to know.
>
> > I can take a look at the Persistence topic at some point.
>
> Sounds good! The last paragraph above might be relevant here. Hope
this works with the persistence classes.
Yes, I noticed this. We'll see.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21272#comment:27>
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