search

AmericanRedCross / street-view-green-view

BSD 3-Clause "New" or "Revised" License
15 stars 15 forks source link

review what triggers a retry image download #54

Open danbjoseph opened 3 months ago

danbjoseph commented 3 months ago

leftover from #52

@jayqi: Looking at how the exceptions are documented and defined:

I think if we're being more judicious, we'd want to retry on: ConnectionError, Timeout, and ChunkedEncodingError for connection errors. Some of the other ones, like Invalid*, indicate a problem with the request content and probably wouldn't be fixed on a retry.

Probably also would be best to be more judicious on which HTTP status codes we retry on. In most cases, 400 errors wouldn't be fixed on a retry, but 500 errors may be.

@dragonejt: While I agree that there are cases where a retry may not change anything, I also don't see any cases where a retry would actively disrupt our logic flow, so these exceptions should be "safe" to retry even if they still fail. There are also cases where, for example, the Mapillary API times out but actually returns an HTTP 400 Bad Request. I agree that we can punt this to an issue to determine the best exceptions to retry on later.

jayqi commented 3 months ago

Copying my response on the rationale for why we want to fix this here for visibility:

It's not just about "safe" from runtime errors. Repeating requests that are known to fail (if request is invalid, or 403 or 404 errors), then we (a) will cause the user to waste a bunch of time making repeated failed requests and waiting for the exponential backoff, and (b) incur unnecessary server load on Mapillary, which is bad etiquette.

https://github.com/AmericanRedCross/street-view-green-view/pull/52#issuecomment-2179747811