-
Notifications
You must be signed in to change notification settings - Fork 53
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-16984: Implement request timeouts #150
base: master
Are you sure you want to change the base?
Conversation
if not config_request_timeout: | ||
return READ_TIMEOUT_SECONDS | ||
|
||
if isinstance(config_request_timeout, int): # pylint: disable=no-else-return |
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.
Why we are considering int instead of float?
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.
MySql does not support float values. Document: https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_net_read_timeout.
Added the same comment at line no. 35
tap_mysql/sync_strategies/common.py
Outdated
This function checks whether the URLError 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a nested function?
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.
Used 1 function for giveup condition.
|
||
return gen_fn | ||
|
||
def reconnect(details): |
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.
why we need this? please add code comment
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 comment in the code.
if isinstance(config_request_timeout, int): # pylint: disable=no-else-return | ||
# return value from config | ||
return config_request_timeout | ||
elif isinstance(config_request_timeout, str) and config_request_timeout.isdigit(): |
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.
@harshpatel4crest Why is this check not similar to other taps where we check that, elif config_request_timeout and float(config_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.
MySQL does not support float values for net_read_timeout
variable. Document: https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_net_read_timeout. Hence only accepted int and int values in string
Added the same comment at line no. 35
Description of change
TDL-16984: Implement request timeouts
QA steps
Risks
Rollback steps