SG-38213 Prevent unexpected retries on error #379
Conversation
It seems this concern was already identified before. If any regression happened, that can help us identify better use cases (and we can always let the customers use the previous versions). |
Co-authored-by: Julien Langlois <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refines retry behavior in the Python API by logging exception reasons and updating tests to expect no automatic retries on general errors.
- Updated tests to assert a single call and removed sleep interval checks.
- Enhanced
_make_callto log exception messages (e) on failures. - Bumped version in
HISTORY.rstwith a note about changing retry policy.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_client.py | Updated assertion to expect a single HTTP call |
| tests/test_api.py | Removed multi-retry assertions and adjusted log check |
| shotgun_api3/shotgun.py | Changed exception handler to log the error reason |
| HISTORY.rst | Added v3.8.5 entry explaining new retry policy |
Comments suppressed due to low confidence (2)
tests/test_client.py:334
- [nitpick] The assertion message still says "Call is repeated" but the test now expects only one call (no repeat). Consider updating this message to accurately reflect that no retries should occur.
"Call is repeated"
tests/test_api.py:2227
- The test checks the log message but doesn’t verify that the request was only called once. Add an assertion on
mock_request.call_countto ensure general exceptions do not trigger retries.
"Request failed. Reason: not working",
| self._close_connection() | ||
| if attempt == max_rpc_attempts: | ||
| LOG.debug("Request failed. Giving up after %d attempts." % attempt) | ||
| raise | ||
| LOG.debug(f"Request failed. Reason: {e}", exc_info=True) |
There was a problem hiding this comment.
Catching all Exception means non-SSL errors will still trigger retry logic. Limit retry handling to specific SSL errors (e.g. SSLEOFError, SSLHandshakeError) and re-raise other exceptions immediately.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
False, we handle this on the first two except blocks.
carlos-villavicencio-adsk
requested review from
julien-lang and
eduardoChaucaGallegos
Co-authored-by: Julien Langlois <[email protected]>
We want to control when python-api does a retry on error.
SSLEOFErrorSSLHandshakeErrorOn
_make_callfunction.We don't want to retry on a general exceptions (e.g. Timeout or remote disconnection) because we might send a resource modification request (create, batch create, etc) and we can end up duplicating things. It seems this concern was already identified before.