Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Enable retries on saas connectors for failures at the http request level #1376

Merged
merged 8 commits into from
Sep 28, 2022

Conversation

earmenda
Copy link
Contributor

@earmenda earmenda commented Sep 22, 2022

Purpose

Http calls within our saas connectors currently don't support retries. Even though retries are supported at the task node level, we don't want to retry all the work which has already been done within the node.

Rather than relying on requests library for retry logic we implemented ourselves so that it is simple to track retries in our upcoming rate limiting work.

Changes

  • Added a decorator retry_send to send method within AuthenticatedClient
  • Decorator sleeps after any failures according to input backoff_factor and retry_count
  • Configured decorator to retry connection exceptions and a list of status codes(429, 502, 503, 504)
  • Retry logic looks for Retry-After header and sleeps based on that. Logic inspired by https://github.com/urllib3/urllib3/blob/26383f7acfcfd631942bbb579f284c0a6ec84f19/src/urllib3/util/retry.py#L301
  • Added a way to test AuthenticatedClient by mocking requests session and testing different retry scenarios
  • Removed sleep from test_zendesk_task.py which was previously needed due to throttles

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #1290

@earmenda earmenda added enhancement New feature or request run unsafe ci checks Triggers running of unsafe CI checks labels Sep 22, 2022
@earmenda earmenda requested a review from galvana September 22, 2022 18:41
@earmenda earmenda self-assigned this Sep 22, 2022
@earmenda
Copy link
Contributor Author

@galvana this is still a draft but I tested this and this lets us uncomment the sleep from the zendesk tests

In my testing zendesk returns a 429 which includes this header:
'retry-after': '38'
Verified we respect this:
Retrying http request in 38 seconds

And tests pass!

@earmenda earmenda requested a review from adamsachs September 23, 2022 07:23
@earmenda earmenda marked this pull request as ready for review September 23, 2022 07:32
@earmenda
Copy link
Contributor Author

This should be ready for review

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great @earmenda, i think it's a really clean implementation and you've got good test coverage. i like that we're supporting both "client-driven" and "server-driven" (i.e. response-header-based) logic.

i have a few minor comments and questions, but nothing too substantial. my general understanding of this is that it's setting up the infrastructure for the client-driven configuration of the retry logic, but on a quick search, i didn't find the issue for that subsequent work of exposing this functionality in the saas config anywhere -- so i just want to confirm that understanding.

happy to chat about it more offline if that's better.

@earmenda
Copy link
Contributor Author

@adamsachs thanks for the review. i updated the PR with what we discussed

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

just for transparency, @earmenda and i talked offline and clarified that my previous understanding of the changes were slightly off base. the client-driven rate limiting will be added on top of this code, but we'll keep the retry logic as it is here.

@earmenda earmenda merged commit 85e992d into main Sep 28, 2022
@earmenda earmenda deleted the earmenda-retry-throttles-saas-connectors branch September 28, 2022 01:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Retries on Http calls for Saas Connectors
2 participants