-
Notifications
You must be signed in to change notification settings - Fork 20
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
added retry backoff for google-ads unauthorised error #83
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
from singer import utils, metrics | ||
from google.protobuf.json_format import MessageToJson | ||
from google.ads.googleads.errors import GoogleAdsException | ||
from google.api_core.exceptions import ServerError, TooManyRequests | ||
from google.api_core.exceptions import ServerError, TooManyRequests, Unauthorized | ||
from requests.exceptions import ReadTimeout | ||
import backoff | ||
from . import report_definitions | ||
|
@@ -237,6 +237,7 @@ def on_giveup_func(err): | |
giveup=should_give_up, | ||
on_giveup=on_giveup_func, | ||
logger=None) | ||
@backoff.on_exception(backoff.expo, Unauthorized, max_tries=5, jitter=None,logger=None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we append Unauthorized in 1st backoff itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the above exceptions have a on_giveup handler, which is not needed and does not work with this specific exception class hence added a new decorator |
||
def make_request(gas, query, customer_id, config=None): | ||
if config is None: | ||
config = {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import unittest | ||
from unittest.mock import Mock, patch | ||
from tap_google_ads.streams import make_request | ||
from google.api_core.exceptions import InternalServerError, BadGateway, MethodNotImplemented, ServiceUnavailable, GatewayTimeout, TooManyRequests | ||
from google.api_core.exceptions import InternalServerError, BadGateway, MethodNotImplemented, ServiceUnavailable, GatewayTimeout, TooManyRequests, Unauthorized | ||
from requests.exceptions import ReadTimeout | ||
|
||
@patch('time.sleep') | ||
|
@@ -97,6 +97,21 @@ def test_429_too_may_request_error(self, mock_sleep): | |
# Verify that tap backoff for 5 times | ||
self.assertEquals(mocked_google_ads_client.search.call_count, 5) | ||
|
||
def test_401_uauthenticated(self, mock_sleep): | ||
""" | ||
Check whether the tap backoffs properly for 5 times in case of TooManyRequests error. | ||
""" | ||
mocked_google_ads_client = Mock() | ||
mocked_google_ads_client.search.side_effect = Unauthorized("Resource has been exhausted") | ||
|
||
try: | ||
make_request(mocked_google_ads_client, "", "") | ||
except Unauthorized: | ||
pass | ||
Comment on lines
+107
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use self.assertRaises instead of try-catch block |
||
|
||
# Verify that tap backoff for 5 times | ||
self.assertEquals(mocked_google_ads_client.search.call_count, 5) | ||
|
||
def test_read_timeout_error(self, mock_sleep): | ||
""" | ||
Check whether the tap backoffs properly for 5 times in case of ReadTimeout error. | ||
|
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.
looking at the error in CCI logs, the import has to be
Unauthenticated