introduce retry for fetching data source configurations

[thilak009]

Changes proposed

• Introduce retry functionality for fetching data source configurations
• configurable through environment variable DATA_STORE_CONN_RETRY

Check List (Check all the applicable boxes)

• I sign off on contributing this submission to open-source
• My code follows the code style of this project.
• My change requires changes to the documentation.
• I have updated the documentation accordingly.
• All new and existing tests passed.
• This PR does not contain plagiarized content.
• The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

[netlify]

✅ Deploy Preview for opal-docs ready!

Name	Link
🔨 Latest commit	36008f8
🔍 Latest deploy log	https://app.netlify.com/sites/opal-docs/deploys/6578818415bbde000873e765
😎 Deploy Preview	https://deploy-preview-502--opal-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[asafc]

Hi @thilak009 :)

overall it looks good but it affects every data fetch from the DataFetcher not just the base configuration. I'd like to make sure @roekatz is at least aware and ok with this change.

I've put a task on him to review.
(@roekatz for your reference: https://linear.app/permit/issue/PER-8094/review-opal-pr-502)

[roekatz]

Hi @thilak009,

Hmm... the FetchingEngine being used by the data fetcher already has a retry mechanism - look for BaseFetchProvider.DEFAULT_RETRY_CONFIG.
Does it not actually take effect?
It's also doesn't seem to be configurable... So your contribution is a good idea - But I would much rather it make use of the existing mechanism.
See FetchingEngine.queue_url - when an FetchEvent is initiated, you can provide retry configuration.
It would also be preferable because I plan on merging a change where the actual data fetches would occur asynchronously.

[thilak009]

Hi @thilak009,

Hmm... the FetchingEngine being used by the data fetcher already has a retry mechanism - look for BaseFetchProvider.DEFAULT_RETRY_CONFIG. Does it not actually take effect? It's also doesn't seem to be configurable... So your contribution is a good idea - But I would much rather it make use of the existing mechanism. See FetchingEngine.queue_url - when an FetchEvent is initiated, you can provide retry configuration. It would also be preferable because I plan on merging a change where the actual data fetches would occur asynchronously.

Hm, i see that a FetchEvent has an option to pass in retry configuration
but the queue_fetch_event method only raises either a asyncio.QueueFull or a asyncio.TimeoutError exception and hence retry wouldn't happen for a bad status code

I see that the underlying class which makes the HTTP requests in fetcher is HttpFetchProvider
does editing the _fetch_ to raise a HTTP bad status code exception make sense if we want to use the existing retry mechanism of FetchEvent ?

[roekatz]

Hi @thilak009,
Hmm... the FetchingEngine being used by the data fetcher already has a retry mechanism - look for BaseFetchProvider.DEFAULT_RETRY_CONFIG. Does it not actually take effect? It's also doesn't seem to be configurable... So your contribution is a good idea - But I would much rather it make use of the existing mechanism. See FetchingEngine.queue_url - when an FetchEvent is initiated, you can provide retry configuration. It would also be preferable because I plan on merging a change where the actual data fetches would occur asynchronously.

Hm, i see that a FetchEvent has an option to pass in retry configuration but the queue_fetch_event method only raises either a asyncio.QueueFull or a asyncio.TimeoutError exception and hence retry wouldn't happen for a bad status code

I see that the underlying class which makes the HTTP requests in fetcher is HttpFetchProvider does editing the _fetch_ to raise a HTTP bad status code exception make sense if we want to use the existing retry mechanism of FetchEvent ?

The queue_fetch_event only puts the fetch request in queue.
The queue is handled by fetch_worker which reads from that queue, then calls get_fetcher_for_event which creates a new fetch provider and sets the retry configuration from the FetchEvent object itself (by using BaseFetchProvider.set_retry_config).
The retry then, AFAIU, would occur for any kind of Exception.

[thilak009]

The retry then, AFAIU, would occur for any kind of Exception.

i don't think so, for a bad status code let's say for a 5XX response, the queue_fetch_event does not raise any exception for this scenario, which means if i understand correctly fetch_worker or any other part which is handling the HTTP call does not raise an exception for 5XX status code
tenacity retry only does a retry if an exception is raised and hence in this case, retry does not happen
am i missing something here?

[roekatz]

am i missing something here?

The queue_fetch_event isn't the method doing the actual fetch, but rather just queues it to be later done asynchronously by the fetch_worker - as I've detailed above.

Therefore, as you said, no exception raised during the fetch can or should be caught there (in queue_fetch_event). Instead, the already implemented retry wraps the actual fetch method in BaseFetchProvider. It simply uses the retry configuration from FetchEvent, that configuration is set in queue_fetch_event.

[obsd]

Closing for now

[thilak009]

Got some time this weekend to take a stab at this again.

So i tried setting retry configuration to the FetchEvent inside the queue_url where we are creating the fetch event. Like so

event = FetchEvent(url=url, fetcher=fetcher, config=config, retry=self._retry_config)

which creates a new fetch provider and sets the retry configuration from the FetchEvent object itself (by using BaseFetchProvider.set_retry_config).
The retry then, AFAIU, would occur for any kind of Exception.

based on what u mentioned, doing this should have triggered a retry when the actual fetch call fails with a status code of 5XX, but retry is not happening.
pardon my knowledge on the codebase but i am having a hard time trying to figure this out

[roekatz]

@thilak009
Oh, now I see what you meant by HttpFetchProvider._fetch_ not raising an exception for a bad status code.
Sorry for overlooking that crucial bit...

So yes - it should be edited to raise an exception, I would recommend simply passing raise_for_status=True either when constructing ClientSession (in __aenter__) or directly when calling the http_method in _fetch_ itself.
Then in combination with the existing retry mechanism we've discussed, the desired behavior should be achieved.
And having the option to override the default retry configuration would be a great addition.

Thanks for finding the time to come back to this! 🙌

[thilak009]

@roekatz i have made the changes as you suggested by setting the raise_for_status for the ClientSession and now the retry works as expected with just passing a retry config to the FetchEvent.
thank you for taking out time reviewing and guiding around the codebase

You can squash commits i guess, the changes are minimal but there are more commits due to the initial implementation 😄

[roekatz]

@thilak009 - That's great, thank you!
The point regarding the reraise isn't clear to me, other than that the code looks good.
Waiting for your answer than I can approve & merge :)