Opened 4 years ago

Closed 3 years ago

#20516 closed enhancement (fixed)

CollecTor's exitlists module should avoid httpurlconnection

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.2.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: metrics-help
Cc: iwakeh Actual Points:
Parent ID: #8799 Points:
Reviewer: Sponsor:


See #20323:

ExitListDownloader should replace HttpURLConnections.
This a little trickier than the other two *Downloader tasks because the annotation needs to be prepended.

Child Tickets

Change History (6)

comment:1 Changed 4 years ago by iwakeh

See also #20543.

comment:2 Changed 4 years ago by iwakeh

Milestone: CollecTor 1.2.0

comment:3 Changed 3 years ago by iwakeh

Cc: iwakeh added

Adding myself to cc to make trac mail updates.

comment:4 Changed 3 years ago by iwakeh

Owner: set to iwakeh
Status: newaccepted

comment:5 Changed 3 years ago by iwakeh

Status: acceptedneeds_review

Simply switching to file copy would mean a major change here as the current download approach reads the data to memory for verifying and after that writes to file(s).
A toy example of an http-server that accepts connections, but never writes the response code yielded the stack-trace provided in #22602. The easiest remedy is setting a read-timeout.
(A quick inspection of the code that gets used when using Files.copy indicates that a timeout seems to be set there, too.)

Open points:

  • the current read timeout is arbitrarily set to 5000ms. How to find the best value?
  • Should a connect timeout be set, too? There is no bug report that indicates necessity for this timeout type.

Please review this change.

comment:6 Changed 3 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Patch looks good and is now merged to master! I believe 5000ms is as good as a first guess as any other value. We'll find out if it's too low or high over time, and we'll find out if we should have set a connect timeout as well. I'd say let's start with this, it's better than what we did before. Closing as fixed. Thanks!

Note: See TracTickets for help on using tickets.