feat(675): Add retry strategy support for all API calls made to Facebook.#691
Open
PacificGilly wants to merge 6 commits intofacebook:mainfrom
Open
feat(675): Add retry strategy support for all API calls made to Facebook.#691PacificGilly wants to merge 6 commits intofacebook:mainfrom
PacificGilly wants to merge 6 commits intofacebook:mainfrom
Conversation
This is needed since the addition of a Retry strategy which is provided by the `urllib3` library. This supports both PY2 (if we're really honestly still supporting this) and PY3 and was added since v1.9.0, so just pinning this to that version just in case someone is using an ancient (11+ years old) version of urllib3.
To provide consistency with how an API call can be configured in this library, wanted to add the ability to configure a blanket retry strategy at the config level if a user wants all API calls (not POST etc.) to have a retry strategy.
The alternative way to handle retries is to specify your own at the session level. This provides full control over how to do retries, but more at the owners own risk (as we've already provided them with a default solution).
Add in the injection point to the FacebookAdsApi to allow manually passing in a custom retry strategy which is then passed into the FacebookSession and finally into the requests.Session object.
Uses a new test library, responses, which allows you to better test making multiple API calls and therefore testing your Retry strategy actually works.
|
@stcheng @satwikareddy3 @nestorhdz-meta @shuaiwa-meta @cnukaus @rodrigobeckmann (contributors in 12 months. sorry for noisy broadcasting) |
|
@stcheng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@stcheng Thank you for importing this pull request. There seems to be something wrong the internal tests but we can't see the reports. If it does matter to merge, please deal with the error or show us the report. Otherwise, please close this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #675, there are occasions where an API call will fail due to server-side issues. It's common practice to retry API calls when they're safe, e.g., GET or POST-type calls with idempotency. Also if too many requests are being made and a 429 is being raised, we want to re-send the request again in a safe manner at the correct time, according to any HTTP headers.
So, adding in a Retry strategy that works well with the FacebookSession class and is written in a consistent form to other features you can specify, like API_VERSION or Timeout.
To make it easier for end users to use this feature, there is a default Retry strategy that can be toggled on via the Ads App Config settings. This default strategy is also partially customisable. Finally, advanced users can specify their own Retry strategy from scratch, but all require the Retry class from urllib3.