DBFS API 429 retries: Fix bug on computing the time until the next retry #326
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
This reverts commit 23cdb52.
… Small additionaly improvements.
bogdanghita-db
commented
Sep 22, 2020
| @@ -113,7 +110,6 @@ def wrapped_function(*args, **kwargs): | |||
| return func(*args, **kwargs) | |||
| except HTTPError as e: | |||
| if e.response.status_code == 429: | |||
| click.echo("Rate limit exceeded. Retrying with exponential backoff.") | |||
There was a problem hiding this comment.
We already have the following logs, no need for this extra one.
Received 429 REQUEST_LIMIT_EXCEEDED for attempt 1. Retrying in 0.91 seconds.
Received 429 REQUEST_LIMIT_EXCEEDED for attempt 2. Retrying in 0.12 seconds.
Received 429 REQUEST_LIMIT_EXCEEDED for attempt 3. Retrying in 2.95 seconds.
Received 429 REQUEST_LIMIT_EXCEEDED for attempt 4. Retrying in 5.58 seconds.
Received 429 REQUEST_LIMIT_EXCEEDED for attempt 5. Retrying in 12.54 seconds.
gotibhai
reviewed
Sep 23, 2020
tests/dbfs/test_api.py
Outdated
| @@ -40,6 +40,7 @@ | |||
| 'file_size': 1 | |||
| } | |||
| TEST_FILE_INFO = api.FileInfo(TEST_DBFS_PATH, False, 1) | |||
| MAX_RETRY_429_ATTEMPTS = 8 | |||
There was a problem hiding this comment.
nit: Consistency with api.py and keep it as MAX_RETRY_ATTEMPTS
gotibhai
reviewed
Sep 23, 2020
| @@ -94,13 +86,21 @@ class DbfsErrorCodes(object): | |||
| def before_sleep_on_429(retry_state): | |||
| global time_for_last_retry | |||
There was a problem hiding this comment.
Is this still needed now? This is using the global declaration. We can remove this since you're now initializing locally everytime now.
There was a problem hiding this comment.
Yes, we need it, since the function is called multiple times and time_for_last_retry needs to be incremented every time. Otherwise, on attempt 2 time_for_last_retry will be uninitialised and we'll get: local variable 'time_for_last_retry' referenced before assignment.
There was a problem hiding this comment.
Ah of course! The same logic still holds. Gotcha!
gotibhai
approved these changes
Sep 23, 2020
bogdanghita-db
added a commit
to bogdanghita-db/databricks-cli
that referenced
this pull request
Oct 16, 2020
… next retry (databricks#326)" This reverts commit 37d51b1.
bogdanghita-db
added a commit
that referenced
this pull request
Oct 20, 2020
…all databricks-cli operations (#343) Revert retry logic on 429 responses for DBFS API requests (#319, #326, #327) and reimplement it using [urllib3.util.Retry](https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html), for all the requests made by `databricks-cli`. * We perform a maximum number of 6 retries with exponential backoff, resulting in the following delays between them: 0.5, 1, 2, 4, 8, 16 (seconds). The `Retry-After` header is respected if present. * There is no message logged for each retry (I could not find a way to do it using `urllib3.util.Retry`). * If all retry attempts fail, the original 429 http response is forwarded by the retry utility.
onlinehub0808
added a commit
to onlinehub0808/databricksCli
that referenced
this pull request
Feb 1, 2023
…all databricks-cli operations (#343) Revert retry logic on 429 responses for DBFS API requests (databricks/databricks-cli#319, databricks/databricks-cli#326, databricks/databricks-cli#327) and reimplement it using [urllib3.util.Retry](https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html), for all the requests made by `databricks-cli`. * We perform a maximum number of 6 retries with exponential backoff, resulting in the following delays between them: 0.5, 1, 2, 4, 8, 16 (seconds). The `Retry-After` header is respected if present. * There is no message logged for each retry (I could not find a way to do it using `urllib3.util.Retry`). * If all retry attempts fail, the original 429 http response is forwarded by the retry utility.
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.
In #319 we added 429 retries for the DBFS API.
Computing the time until the next retry as
retry_state.idle_for - time_for_last_retryis not going to work as expected in the case where more than 1 call to functions wrapped withretry_429happens (e.g. in the case ofput_file). The problem is thattime_for_last_retryis only initialised once per databricks-cli command, while it should be initialised every time a function wrapped withretry_429is called, in the first attempt.We fix this by initialising
time_for_last_retryifretry_state.attempt_number == 1. This only works if the code is single-threaded, which is the case here.Additional improvements:
retry_state.idle_for.