-
Notifications
You must be signed in to change notification settings - Fork 3
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
HGI-5432 \ Add jitter to always wait 120s when retrying #10
base: master
Are you sure you want to change the base?
Conversation
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.
Hey! Great work. Left a request to improve security in the way that we access attributes on the response.
tap_hubspot/__init__.py
Outdated
if exc.response is not None and exc.response.status_code != 429: | ||
# this is the only handler in backoff that contains the status_code in its parameters | ||
time_avoid_rate_limit_in_seconds = 30 | ||
time.sleep(time_avoid_rate_limit_in_seconds) | ||
return False | ||
|
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.
if exc.response is not None and exc.response.status_code != 429: | |
# this is the only handler in backoff that contains the status_code in its parameters | |
time_avoid_rate_limit_in_seconds = 30 | |
time.sleep(time_avoid_rate_limit_in_seconds) | |
return False | |
# Check if the exception has a response attribute with a status_code | |
if not getattr(exc, 'response', None) or exc.response is None: | |
return False | |
status_code = getattr(exc.response, 'status_code', None) | |
if status_code is None: | |
LOGGER.warning("Exception response has no status_code.") | |
return False | |
# If the status code is not 429 (rate limit) sleep and continue | |
if status_code != 429: | |
time_avoid_rate_limit_in_seconds = 30 | |
LOGGER.info(f"Sleeping for {time_avoid_rate_limit_in_seconds} seconds due to status code {status_code}.") | |
time.sleep(time_avoid_rate_limit_in_seconds) | |
return False | |
# If the status code is in the range of 400 to 499 | |
if 400 <= status_code < 500: | |
LOGGER.info(f"Non-retriable error with status code {status_code}.") | |
return True | |
return False | |
tap_hubspot/__init__.py
Outdated
return exc.response is not None \ | ||
and 400 <= exc.response.status_code < 500 \ | ||
and exc.response.status_code != 429 | ||
and 400 <= exc.response.status_code < 500 |
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.
If you accept my suggestion above, you should delete this return
# Check if the exception has a response attribute with a status_code | ||
if not getattr(exc, 'response', None) or exc.response is None: | ||
return False | ||
|
||
status_code = getattr(exc.response, 'status_code', None) | ||
|
||
if status_code is None: | ||
LOGGER.warning("Exception response has no status_code.") | ||
return False | ||
|
||
# If the status code is not 429 (rate limit) sleep and continue | ||
if status_code == 429: | ||
time_avoid_rate_limit_in_seconds = 30 | ||
LOGGER.info(f"Sleeping for {time_avoid_rate_limit_in_seconds} seconds due to status code {status_code}.") | ||
time.sleep(time_avoid_rate_limit_in_seconds) | ||
return False | ||
|
||
# If the status code is in the range of 400 to 499 | ||
if 400 <= status_code < 500: | ||
LOGGER.info(f"Non-retriable error with status code {status_code}.") | ||
return True | ||
|
||
return False |
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.
# Check if the exception has a response attribute with a status_code | |
if not getattr(exc, 'response', None) or exc.response is None: | |
return False | |
status_code = getattr(exc.response, 'status_code', None) | |
if status_code is None: | |
LOGGER.warning("Exception response has no status_code.") | |
return False | |
# If the status code is not 429 (rate limit) sleep and continue | |
if status_code == 429: | |
time_avoid_rate_limit_in_seconds = 30 | |
LOGGER.info(f"Sleeping for {time_avoid_rate_limit_in_seconds} seconds due to status code {status_code}.") | |
time.sleep(time_avoid_rate_limit_in_seconds) | |
return False | |
# If the status code is in the range of 400 to 499 | |
if 400 <= status_code < 500: | |
LOGGER.info(f"Non-retriable error with status code {status_code}.") | |
return True | |
return False | |
# Check if the exception has a response attribute with a status_code | |
if not getattr(exc, 'response', None) or exc.response is None: | |
return False | |
status_code = getattr(exc.response, 'status_code', None) | |
if status_code is None: | |
LOGGER.warning("Exception response has no status_code.") | |
return False | |
# If the status code is not 429 (rate limit) sleep and continue | |
if status_code == 429: | |
time_avoid_rate_limit_in_seconds = 30 | |
LOGGER.info(f"Sleeping for {time_avoid_rate_limit_in_seconds} seconds due to status code {status_code}.") | |
time.sleep(time_avoid_rate_limit_in_seconds) | |
return False | |
# If the status code is in the range of 400 to 499 | |
if 400 <= status_code < 500: | |
LOGGER.info(f"Non-retriable error with status code {status_code}.") | |
return True | |
return False |
I messed with the indentation level before! Now it might be fixed 🙏
Description of change
contact/v1/lists
endpoint was raising 429 error and excepted after the 5 retries. Add jitter to guarantee 120s between retriesManual QA steps
Risks
Rollback steps