Always retry on HTTP 429 and 503, even if Retry-After header is not available
#396
Conversation
This file contains 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
…ists`, `BadRequest`, `PermissionDenied`, `InternalError`, and others Improve the ergonomics of SDK, where instead of `except DatabricksError as err: if err.error_code != 'NOT_FOUND': raise err else: do_stuff()` we could do `except NotFound: do_stuff()`. Additionally, it'll make it easier to read stack traces, as they will contain specific exception class name. # First principles - do not override `builtins.NotImplemented` for `NOT_IMPLEMENTED` error code - assume that platform error_code/HTTP status code mapping is not perfect and in the state of transition - we do represent reasonable subset of error codes as specific exceptions ## Open questions ### HTTP Status Codes vs Error Codes 1. Mix between status codes and error codes (preferred) 2. Rely only on HTTP status codes 3. Rely only on `error_code`'s One example is `BAD_REQUEST` error code that maps onto HTTP 400 and to `except BadRequest as err` catch clause. But `MALFORMED_REQUEST`, `INVALID_STATE`, and `UNPARSEABLE_HTTP_ERROR` do also map to HTTP 400. So the proposal is to remap the MALFORMED_REQUEST to `BadRequest` exception. Another corner-case is UC: - 'METASTORE_DOES_NOT_EXIST': NotFound, - 'DAC_DOES_NOT_EXIST': NotFound, - 'CATALOG_DOES_NOT_EXIST': NotFound, - 'SCHEMA_DOES_NOT_EXIST': NotFound, - 'TABLE_DOES_NOT_EXIST': NotFound, - 'SHARE_DOES_NOT_EXIST': NotFound, - 'RECIPIENT_DOES_NOT_EXIST': NotFound, - 'STORAGE_CREDENTIAL_DOES_NOT_EXIST': NotFound, - 'EXTERNAL_LOCATION_DOES_NOT_EXIST': NotFound, - 'PRINCIPAL_DOES_NOT_EXIST': NotFound, - 'PROVIDER_DOES_NOT_EXIST': NotFound, ### Naming conflict resolution We have three sources of naming: - `error_code` - HTTP Status - Python builtin exceptions We still have to define which name takes the precedence.
… available This PR improves retry logic by using more type-safe code to determine sleep time periods between retries or falling back to the default sleep interval.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 13, 2023
## Changes Change HTTP client to always retry on 429 and 503, regardless of whether Retry-After header is present. If absent, default to 1 second, as we do today. Subsumes #391 and #396 while we're still discussing error architecture in the SDK. ## Tests Unit test to ensure that 429 and 503 work with and without Retry-After header.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
This PR improves retry logic using more type-safe code to determine sleep periods between retries or falling back to the default sleep interval. Relies on concrete exception types introduced by #376
Tests
make testrun locallymake fmtapplied