TDL-16648: Implement request timeouts #129
Conversation
tap_shopify/streams/base.py
Outdated
| self.request_timeout = REQUEST_TIMEOUT # set default timeout | ||
| timeout_from_config = Context.config.get('request_timeout') | ||
| # updated the timeout value if timeout is passed in config and not from 0, "0", "" | ||
| if timeout_from_config and float(timeout_from_config): |
There was a problem hiding this comment.
Why do we need to write this code 2 times, can't we put this inside one function and use it?
There was a problem hiding this comment.
Added a function that returns the timeout value.
| # verify the timeout is set as expected | ||
| self.assertEquals(stream.request_timeout, 100) | ||
|
|
||
| def test_timeout_empty_value_passed_in_config(self): |
There was a problem hiding this comment.
Please add function comments
tap_shopify/streams/base.py
Outdated
| This function checks whether the error contains 'timed out' substring and return boolean | ||
| values accordingly, to decide whether to backoff or not. | ||
| """ | ||
| def gen_fn(exc): |
There was a problem hiding this comment.
do we need inner function? OR one function is enough
There was a problem hiding this comment.
Used 1 function for giveup condition.
|
|
||
| @shopify_error_handling |
There was a problem hiding this comment.
Why do we remove this, isn't it required for other error handling?
There was a problem hiding this comment.
As we have added the decorator over the find method. Hence removed this decorator.
|
|
||
| @shopify_error_handling |
There was a problem hiding this comment.
Why do we remove this, isn't it required for other error handling?
There was a problem hiding this comment.
As we have added the decorator over the find method. Hence removed this decorator.
| @backoff.on_exception(backoff.expo, # timeout error raise by Shopify | ||
| (pyactiveresource.connection.Error, socket.timeout), | ||
| giveup=is_timeout_error, | ||
| max_tries=MAX_RETRIES, |
There was a problem hiding this comment.
@harshpatel4crest Don't we have to use retry_after_wait_gen function to wait for the next retry?
There was a problem hiding this comment.
In the code, retry_after_wait_gen is used for the request error that contains the header in the error response.
There was a problem hiding this comment.
Ok, so retry_after_wait_gen will be used in case of Errors like 429 (Max retries). Got it, Thank you
| path: test_output/report.xml | ||
| - store_artifacts: | ||
| path: htmlcov | ||
| - add_ssh_keys |
There was a problem hiding this comment.
I don't think this add_ssh_keys line is necessary
* added request timeout * resolve pylint error * resolve integration test failure * resolve pylint error * changed 5 with MAX_RETRIES * added request timeout in discovery call * added socket timeout error backoff * created a function to return timeout value * updated the backoff to use 1 function for giveup * resolve pylint error
Description of change
TDL-16648: Implement request timeouts
QA steps
Risks
Rollback steps