#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 karsten):
Replying to [comment:17 iwakeh]:
> From reading the git diffs, I have a few questions and suggestions:
Wow, quick review there. Thanks!
> Does this code reflect the transition period of processing both
> torperf and onionperf files? Is that the reason for introducing
> the empty property with the meaning of 'nothing to download'?
Huh, good point. What I had in mind that we could use this code to 0) run
the same configuration as we do now, 1) start collecting OnionPerf files,
and soon after 2) stop collecting Torperf files. But we could achieve the
same by deploying this code when we do 1) and deploy another patch for 2).
I can undo that change.
> Surely tests will be added for the new functionality of
`Configuration`'s
> `getStringArray` and `getStringArrayArray` methods?
> I assume current code that uses these methods relies on receiving a non-
empty
> array or double array. It needs to be verified that we don't run
> into trouble in the existing code using these methods.
Right. Let me undo this part of the change.
> All storing of files should be done by the persist-mechanism, i.e.,
> a `o.t.c.persist.TpfPersistence` class needs to be introduced (this
would
> shorten `TorperfDownloader` quite a bit and prevent another
> re-implementation of this functionality).
> And, this is a prerequisite for also syncing these files with
> CollecTor's sync-mechanism, which was left out because we knew
> a Torperf replacement is coming.
Hmm, is this something you might be able to do? I'd really want us to
deploy this code before Amsterdam, so that we can discuss next steps
there. Otherwise, how about we defer this part until after Amsterdam?
> It might be useful to put the Configuration's getUrlArray method
> to work. The old Torperf code wasn't modernized, b/c we knew
> it will be replaced. Now, it might be better to have
> OnionPerfHosts as Url array property. This way the url-property
> checking in the downloader class is not necessary anymore as
`Configuration`
> already provides it.
> Why is the host-nick-name needed and could not be replaced by the
hostname
> itself? (For torperf it is used for further configuration.) Or, is
this
> one of the rough edges and note yet available?
> If so, maybe have a different configuration approach instead of the
hybrid-String-Url array?
> It would be great to make the configuration simpler to read
> and edit and keep allmost all property-checking code in the
> `Configuration` class. For example, an url-array and a string-array-
array
> property, the latter for the configuration similar to torperf.
> The only check left for the TorperfDownloader would be to match
> length of these two array- properties, but there might be other
> good approaches.
Well, my idea was that we should use the configured source name to make
sure that an OnionPerf host doesn't send us measurement results for other
sources, either accidentally or on purpose. I'm not sure how we could use
the hostname there if we want to stick to the
[https://collector.torproject.org/recent/torperf/ current naming schema of
.tpf files]. But it could be that I overlooked something.
However, if we want to keep this, what we could do is add a new method
`Configuration.getStringUrlMap()` that returns a (sorted) map of Strings
to URLs. Again, would you want to write such a thing?
Thanks again!
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21272#comment:18>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs
