Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increase default retry delay #1514

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

altano
Copy link

@altano altano commented Oct 30, 2023

Summary

Creates new defaults for retrying delays:

Thing Old Value New Value Reasoning
DEFAULT_MAX_RETRIES 3 12 2^12 seconds is just over 1 hour, which should be enough for OneNote
MAX_DELAY 180 seconds 3_600 (1 hour) This makes it so that retry 12+ only wait 1 hour between retries instead of continuing to backoff the delay exponentially
MAX_MAX_RETRIES 10 64 This needs to be higher than DEFAULT_MAX_RETRIES but the choice of 64 is arbitrary

Motivation

Some of the graph APIs do not provide the retry-after header in HTTP 429 throttling responses. The default behavior of this SDK is to retry 3 times, with exponential backoff. The problem with this approach is that the max retry of 3 attempts (with a delay of ~8 seconds) just isn't enough for some APIs. For example, OneNote's API will potentially return 429s for 1 hour.

See this GitHub issue for more discussion.

Test plan

I was hoping to write unit tests for this but I can't get the tests to run (#1513).

Closing issues

Fixes #978

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Some of the graph APIs do not provide the `retry-after` header in HTTP 429 throttling responses. The default behavior of this SDK is to retry 3 times, with exponential backoff. The problem with this approach is that the max retry of 3 attempts (with a delay of ~8 seconds) just isn't enough for some APIs. For example, OneNote's API will potentially return 429s for 1 hour.

To fix this, let us create some new defaults:

| Thing               | Old Value   | New Value      | Reasoning                                                                                                                 |
| ------------------- | ----------- | -------------- | ------------------------------------------------------------------------------------------------------------------------- |
| DEFAULT_MAX_RETRIES | 3           | 12             | 2^12 seconds is just over 1 hour, which should be enough for OneNote                                                      |
| MAX_DELAY           | 180 seconds | 3_600 (1 hour) | This makes it so that retry 12+ only wait 1 hour between retries instead of continuing to backoff the delay exponentially |
| MAX_MAX_RETRIES     | 10          | 64             | This needs to be higher than DEFAULT_MAX_RETRIES but the choice of 64 is arbitrary                                        |

See [this GitHub issue](microsoftgraph#978) for more discussion.
@altano altano requested a review from a team as a code owner October 30, 2023 02:26
@altano
Copy link
Author

altano commented Oct 30, 2023

@microsoft-github-policy-service agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK doesn't appear to use Retry-After on 429s
1 participant