- Notifications
You must be signed in to change notification settings - Fork 204
SG-11528 Add optional sleep between retries#196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
15 commits Select commit Hold shift + click to select a range
9de6c55 Add rpc_attempt_interval to config
willis102 9d217a0 Update expected preferences to include duration/day and view_master_s…
willis102 d8d790a Fix whitespace.
willis102 6d8a3c3 Allow setting retry interval by passing parameter
willis102 2f2192a Minor cleanup for clarity.
willis102 1103ba0 Make float division easier to read.
willis102 c610655 Wrap retry_interval tests in try-finally
willis102 f2140b8 Remove parameter, and add better comment
willis102 6645608 Give a more useful warning when an invalid value is set
willis102 5c7a9f1 Update view_master_settings field in expected preferences.
willis102 b763f26 Ensure rpc_attempt_interval is positive.
willis102 bf585ce Use trusty dist for travis, since xenial doesn't support python 2.6
willis102 769cb3c Packaging for the v3.0.41 release.
willis102 9c6afd7 Add documentation for environment variables.
willis102 5810c21 Fix spelling in documentation.
willis102 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 file contains hidden or 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 file contains hidden or 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 file contains hidden or 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 file contains hidden or 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 file contains hidden or 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 file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -5,6 +5,7 @@ | ||
| import base64 | ||
| import datetime | ||
| import urllib | ||
| import os | ||
| import re | ||
| try: | ||
| import simplejson as json | ||
| @@ -225,15 +226,61 @@ def test_connect_close(self): | ||
| self.sg.close() | ||
| self.assertEqual(None, self.sg._connection) | ||
| def test_network_retry(self): | ||
| """Network failure is retried""" | ||
| """Network failure is retried, with a sleep call between retries.""" | ||
| self.sg._http_request.side_effect = httplib2.HttpLib2Error | ||
| self.assertRaises(httplib2.HttpLib2Error, self.sg.info) | ||
| self.assertTrue( | ||
| self.sg.config.max_rpc_attempts ==self.sg._http_request.call_count, | ||
| "Call is repeated") | ||
| with mock.patch("time.sleep") as mock_sleep: | ||
| self.assertRaises(httplib2.HttpLib2Error, self.sg.info) | ||
| self.assertTrue( | ||
| self.sg.config.max_rpc_attempts == self.sg._http_request.call_count, | ||
| "Call is repeated") | ||
| # Ensure that sleep was called with the retry interval between each attempt | ||
| attempt_interval = self.sg.config.rpc_attempt_interval / 1000.0 | ||
| calls = [mock.callargs(((attempt_interval,),{}))] | ||
| calls *= (self.sg.config.max_rpc_attempts - 1) | ||
| self.assertTrue( | ||
| mock_sleep.call_args_list == calls, | ||
| "Call is repeated at correct interval." | ||
| ) | ||
| def test_set_retry_interval(self): | ||
| """Setting the retry interval through parameter and environment variable works.""" | ||
| original_env_val = os.environ.pop("SHOTGUN_API_RETRY_INTERVAL", None) | ||
willis102 marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| try: | ||
| def run_interval_test(expected_interval, interval_property=None): | ||
| self.sg = api.Shotgun(self.config.server_url, | ||
| self.config.script_name, | ||
| self.config.api_key, | ||
| http_proxy=self.config.http_proxy, | ||
| connect=self.connect) | ||
| self._setup_mock() | ||
| if interval_property: | ||
| # if a value was provided for interval_property, set the | ||
| # config's property to that value. | ||
| self.sg.config.rpc_attempt_interval = interval_property | ||
| self.sg._http_request.side_effect = httplib2.HttpLib2Error | ||
| self.assertEqual(self.sg.config.rpc_attempt_interval, expected_interval) | ||
| self.test_network_retry() | ||
| # Try passing parameter and ensure the correct interval is used. | ||
| run_interval_test(expected_interval=2500, interval_property=2500) | ||
| # Try setting ENV VAR and ensure the correct interval is used. | ||
| os.environ["SHOTGUN_API_RETRY_INTERVAL"] = "2000" | ||
| run_interval_test(expected_interval=2000) | ||
| # Try both parameter and environment variable, to ensure parameter wins. | ||
| run_interval_test(expected_interval=4000, interval_property=4000) | ||
| finally: | ||
| # Restore environment variable. | ||
| if original_env_val is not None: | ||
| os.environ["SHOTGUN_API_RETRY_INTERVAL"] = original_env_val | ||
| elif "SHOTGUN_API_RETRY_INTERVAL" in os.environ: | ||
| os.environ.pop("SHOTGUN_API_RETRY_INTERVAL") | ||
| def test_http_error(self): | ||
| """HTTP error raised and not retried.""" | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not your code, but I'm really worried about this
exceptclause. Any errors raises by the code, even programming errors on our part, will retry the request. This seems dangerous to me.@thebeeland , should we log a tech debt ticket about this and figure out how to properly fix it? We'd have to take into account all the different http and socket error types that can be encountered here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should, and I will. Thanks for the heads up.