Fetcher retries

[barbarahui]

• implement retries in base Fetcher class
• move make_http_request() functionality into ucd_json_fetcher -- this is the only place this was being used

[amywieliczka]

Why did you remove the back off factor - that seems useful and relevant?

[amywieliczka]

I also wonder if this makes sense as a global configuration - like in rikolti/utils.py?

[barbarahui]

Well, I didn't actually remove the backoff factor as there wasn't one configured at all here previously. I figured that we could go with the default backoff factor of 0 and tweak if necessary. But sure, it probably makes sense to set it to 2.

Yeah, we could make this a global configuration. This is actually a copy of configure_http_session() from the content harvester code (which doesn't have a backoff factor set).

[amywieliczka]

If you added the backoff_factor to the gloabl configuration for self.http_session, then you should be able to do:

Suggested change

	response = session.get(url=url)
	response = self.http_session.get(url=url)

And get rid of lines 93-95 here.

Sure, self.http_session adds some status codes to the forcelist and attached the retry strategy to both http and https, but that seems fine?

[barbarahui]

Yeah, I thought about that too. I figured to introduce the least amount of change possible, but you're right, it probably would be fine.

[amywieliczka]

Mmmm, that makes sense - what you did change (Fetcher.fetch_page()) is the function that is called like 95% of the time though, so it seemed confusing to me to change it most of the way but not all of the way. Could result in weird errors on strange edge cases where we've had to implement additional requests outside of the standard requests managed by the Fetcher base class.

[amywieliczka]

Looks like nuxeo_fetcher, oac_fetcher, and ucd_json_fetcher all make their own calls to requests.get (at least in the fetcher) so you might want to update those other instances in nuxeo_fetcher and oac_fetcher to use self.http_session.get instead.

Oh, yeah and a global search for Retry across the codebase is actually pretty demonstrative - the Retry configuration is the same across several mappers and a fetcher (islandora_mapper, contentdm_mapper, flickr_fetcher)

Since it does look like we use the same configuration everywhere (with the exception of this one discrepancy on backoff_factor and status_forcelist), I think adding it to a rikolti/utils.py makes a whole lot of sense, unless you can think of some reason why we would want different configuration in these different places?

[barbarahui]

@amywieliczka Sure yeah, I think it makes sense to do this. I can do the work to put this in place. I don't think it'll take too long--I was just trying to do this quickly so that I could focus on the Nuxeo API issue.

[amywieliczka]

Figured I could do this quickly while @barbarahui's head was in Nuxeo API issues.
Resolved my own asks - @barbarahui I'll leave this open in case you want to take a look, but as far as I'm concerned it's good to go - rebase & merge when you get to it.

I did leave us using the default requests session for all requests to our own Registry API, as well as requests to the OpenSearch API in the record_indexer.

[barbarahui]

@amywieliczka this looks great, thank you so much!!!!