update FilesExt retry logic #1211
Conversation
yuanjieding-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
yuanjieding-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
yuanjieding-db
force-pushed
the
fix-indefinite-retry
branch
from
45745d5 to
969720e
Compare
yuanjieding-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
parthban-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
| # Maximum number of retry attempts for FilesExt cloud API operations. | ||
| # This works in conjunction with retry_timeout_seconds - whichever limit | ||
| # is hit first will stop the retry loop. | ||
| files_ext_cloud_api_max_retries: int = 3 |
There was a problem hiding this comment.
Maybe we can use this as a temporary parameter, since our end goal is not to fallback.
| # Maximum number of retry attempts for FilesExt cloud API operations. | |
| # This works in conjunction with retry_timeout_seconds - whichever limit | |
| # is hit first will stop the retry loop. | |
| files_ext_cloud_api_max_retries: int = 3 | |
| # Maximum number of retry attempts for FilesExt cloud API operations. | |
| # This works in conjunction with retry_timeout_seconds - whichever limit | |
| # is hit first will stop the retry loop. | |
| experimental_files_ext_cloud_api_max_retries: int = 3 |
|
|
||
| # Determine which limit was hit | ||
| if max_attempts is not None and attempt > max_attempts: | ||
| raise TimeoutError(f"Exceeded max retry attempts ({max_attempts})") from last_err |
There was a problem hiding this comment.
Do we have a better error to represent this error? TimeoutError feels a bit odd for this case, as the function is not actually timed out.
| def failing_function(): | ||
| nonlocal call_count | ||
| call_count += 1 | ||
| raise ValueError("test error") |
There was a problem hiding this comment.
Can we sleep on this failing function for a sec? I am a bit worried that this test will become flaky because it is possible to retry 100 times in 2 seconds.
| def test_max_attempts_none_preserves_backward_compatibility(): | ||
| """Test that max_attempts=None only uses timeout (backward compatibility).""" |
There was a problem hiding this comment.
| def test_max_attempts_none_preserves_backward_compatibility(): | |
| """Test that max_attempts=None only uses timeout (backward compatibility).""" | |
| def test_max_attempts_none(): | |
| """Test that max_attempts=None only uses timeout.""" |
I think we don't need to mention that this test is for backward compatibility because, almost always, a unit test is for regression catching.
| assert call_count == attempts | ||
|
|
||
|
|
||
| def test_max_attempts_respected(): |
There was a problem hiding this comment.
Can we make these a table test to simplify the tests?
| with pytest.raises(TimeoutError) as exc_info: | ||
| failing_function() | ||
|
|
||
| # Should have attempted 3 times (initial + 2 retries) |
There was a problem hiding this comment.
| # Should have attempted 3 times (initial + 2 retries) | |
| # Should have attempted 3 times (initial + 2 retries). |
Period after a sentence, ditto all.
2 participants
What changes are proposed in this pull request?
WHAT
max_attemptto allow client to retry and fail after certain amountWHY
How is this tested?
Unit tests were updated to reflect the change.