-
Notifications
You must be signed in to change notification settings - Fork 39
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
TDL-15860 Implement Request Timeouts #79
Conversation
@@ -19,7 +19,8 @@ OAuth is the default authentication method for `tap-zendesk`. To use OAuth, you | |||
{ | |||
"access_token": "AVERYLONGOAUTHTOKEN", | |||
"subdomain": "acme", | |||
"start_date": "2000-01-01T00:00:00Z" | |||
"start_date": "2000-01-01T00:00:00Z", | |||
"request_timeout": 300 |
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.
Please add details of it as it is the optional parameter.
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.
added.
.circleci/config.yml
Outdated
- run: | ||
name: 'pylint' | ||
command: | | ||
source /usr/local/share/virtualenvs/tap-zendesk/bin/activate | ||
make test | ||
pylint tap_zendesk -d missing-docstring,invalid-name,line-too-long,too-many-locals,too-few-public-methods,fixme,stop-iteration-return,too-many-branches,useless-import-alias,no-else-return,logging-not-lazy |
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.
@prijendev Did you go through the errors returned by pylint which you have skipped intentionally?
Is it something that cannot be resolved?
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.
make test
command runs both pylint
and nosetests
. But now as we are adding code coverage with unittests we need to change nosetests
with some extra argument. So, it's just spllited make test
command. We have not changed existing pylint command. Existing command can be seen under pylint section of circleci
tap_zendesk/__init__.py
Outdated
# Set request timeout to config param `request_timeout` value. | ||
# If value is 0,"0","" or not passed then it set default to 300 seconds. | ||
config_request_timeout = parsed_args.config.get('request_timeout') | ||
request_timeout = config_request_timeout and float(config_request_timeout) or REQUEST_TIMEOUT # pylint: disable=consider-using-ternary |
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.
@prijendev Please update this condition as follows for better visibility and understanding
request_timeout = (config_request_timeout and float(config_request_timeout)) or REQUEST_TIMEOUT
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.
Updated
response = requests.get(url, params=params, headers=headers) | ||
@backoff.on_exception(backoff.expo,Timeout, #As timeout error does not have attribute status_code, hence giveup does not work in this case. | ||
max_tries=10) # So, here we added another backoff expression. | ||
def call_api(url, request_timeout, params, headers): |
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.
@prijendev I see all the calls to function call_api
is updated due to addition of a single parameter in the function argument.
My suggestion:
Since request_timeout
is initialized in the main()
function, can't we directly utilize it in the call_api
function instead of passing it as an argument? The benefit of this change will be, changes to call_api
function can be removed. Minimal changes will be resulted in the PR. Code would be more robust and generic.
CC: @dbshah1212
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.
In this tap some of the streams using Zenpy sdk. So, to initialize sdk client we have read request_timeout in main()
function. While for other streams each sync method using call_api
function. Updated initialization of request_timeout
to common parent Stream
class to minimize the changes.
tap_zendesk/streams.py
Outdated
for page in http.get_cursor_based(url, self.config['access_token'], **kwargs): | ||
# Set and pass request timeout to config param `request_timeout` value. | ||
# If value is 0,"0","" or not passed then it set default to 300 seconds. | ||
config_request_timeout = self.config.get('request_timeout') |
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.
@prijendev This code is already written in the main()
function of init file. What is the need of initializing these variables again? Configuration parameters should be read only once in the starting of the tap.
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.
In this tap some of the streams using Zenpy sdk. So, to initialize sdk client we have read request_timeout in main() function. While for other streams updated initialization of request_timeout
to common parent Stream class to minimize the changes. So, overall we require to read request_timeout
2 times.
tap_zendesk/streams.py
Outdated
for page in http.get_incremental_export(url, self.config['access_token'], start_time): | ||
# Set and pass request timeout to config param `request_timeout` value. | ||
# If value is 0,"0","" or not passed then it set default to 300 seconds. | ||
config_request_timeout = self.config.get('request_timeout') |
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.
@prijendev Same comment here as well
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.
In this tap some of the streams using Zenpy sdk. So, to initialize sdk client we have read request_timeout in main() function. While for other streams updated initialization of request_timeout to common parent Stream class to minimize the changes. So, overall we require to read request_timeout 2 times.
@@ -13,6 +13,7 @@ | |||
'after_cursor': 'some_cursor'} |
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.
@prijendev Did we write test cases for different scenarios of request_timeout
variable?
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.
Yes, We write test cases for different scenarios of request_timeout variable in test_request_timeout.py file.
tap_zendesk/streams.py
Outdated
# If value is 0,"0","" or not passed then it set default to 300 seconds. | ||
config_request_timeout = self.config.get('request_timeout') | ||
request_timeout = config_request_timeout and float(config_request_timeout) or REQUEST_TIMEOUT # pylint: disable=consider-using-ternary | ||
pages = http.get_offset_based(url, self.config['access_token'], request_timeout) |
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.
Did we write test cases for this change?
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.
Yes, we write test cases for this change in test_request_timeout.py. Method name - test_ticket_metrics_timeout_error_with_empty_value
…com/singer-io/tap-zendesk into TDL-15860-Implement-Request-Timeout
tap_zendesk/streams.py
Outdated
# Set and pass request timeout to config param `request_timeout` value. | ||
# If value is 0,"0","" or not passed then it set default to 300 seconds. | ||
config_request_timeout = self.config.get('request_timeout') | ||
self.request_timeout = (config_request_timeout and float(config_request_timeout)) or REQUEST_TIMEOUT # pylint: disable=consider-using-ternary |
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.
@prijendev We should figure out an alternative to resolve the pylint error. Why to disable it completely?
Can you research on what can be the alternative ways to assign values to self.request_timeout
?
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.
Removed disable of pylint error. Updated code.
tap_zendesk/http.py
Outdated
@@ -22,12 +22,14 @@ def is_fatal(exception): | |||
requests.exceptions.HTTPError, | |||
max_tries=10, | |||
giveup=is_fatal) | |||
def call_api(url, params, headers): | |||
response = requests.get(url, params=params, headers=headers) | |||
@backoff.on_exception(backoff.expo,Timeout, #As timeout error does not have attribute status_code, hence giveup does not work in this case. |
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 are adding max_tries=5 with factor=2 so please update accordingly to give significant delay between 2 retries.
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.
Updated it.
tap_zendesk/__init__.py
Outdated
if config_request_timeout and float(config_request_timeout): | ||
request_timeout = float(config_request_timeout) | ||
else: | ||
request_timeout = REQUEST_TIMEOUT # If value is 0,"0","" or not passed then it set default to 300 seconds. |
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.
request_timeout = REQUEST_TIMEOUT # If value is 0,"0","" or not passed then it set default to 300 seconds. | |
request_timeout = REQUEST_TIMEOUT # If value is 0, "0", "" or not passed then it sets default to 300 seconds. |
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.
Updated.
Description of change
Note :
Manual QA steps
Risks
Rollback steps