SG-38213 Prevent unexpected retries on error #379
Conversation
julien-lang
left a comment
There was a problem hiding this comment.
Looks good but I am worried we are going to create new regressions with removing that retry.
Did you identify when and why this retry on generic exceptions was introduced?
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). |
julien-lang
left a comment
There was a problem hiding this comment.
Just 2 minor points, then I'll be happy :)
Co-authored-by: Julien Langlois <16244608+julien-lang@users.noreply.github.com>
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",
julien-lang
left a comment
There was a problem hiding this comment.
LGTM!
Just one minor comment
Co-authored-by: Julien Langlois <16244608+julien-lang@users.noreply.github.com>
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.