#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: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? > 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. > 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. 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. > 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. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21272#comment:26> 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