TDL-6148: Catch and retry Attribute error and TDL-13267: Fix AdCreative requests that are not retrying #171

savan-chovatiya · 2021-10-11T11:51:20Z

Description of change

TDL-6148: catch and retry 'AttributeError: 'str' object has no attribute 'get''

Added AttributeError in retry_pattern decorator for all the sync related calls.
Also, added AttributeError into should_retry_api_error for catching.

TDL-13267: AdCreative requests that are not retrying

Added retry_pattern decorator on the sync_batches() function of AdCreatives with FacebookRequestError also.

Manual QA steps

Added unit test case for verifying retries on the above exceptions

Risks

Rollback steps

revert this branch

dbshah1212 · 2021-11-12T11:37:10Z

tap_facebook/__init__.py

@@ -153,6 +153,8 @@ def should_retry_api_error(exception):
            return True
        elif isinstance(exception, TypeError) and str(exception) == "string indices must be integers":
            return True
+        elif isinstance(exception, AttributeError):


We can add it in the or of if isinstance(exception, FacebookBadObjectError), instead of adding a new elif.

Updates as per suggestion.

karanpanchal-crest · 2021-11-17T11:13:52Z

tests/unittests/test_sync_batches_retry.py

@@ -0,0 +1,154 @@
+import unittest


@savan-chovatiya What is the difference between the test cases written in this file and test_attribute_error_retry.py file.
In the above file as well, you are checking that call_count should be 5 times. What else you are checking in test_sync_batches_retry file?

There was no backoff on the sync_batches function so I added and also include the FacebookRequestError error with AttributeError error so those tests are under test_sync_batches_retry. While other test cases for AttributeError are under 'test_attribute_error_retry.py'

karanpanchal-crest · 2021-11-17T11:14:26Z

tests/unittests/test_sync_batches_retry.py

+        with self.assertRaises(AttributeError):
+            ad_creative_object.sync_batches([])
+
+        # verify calls inside sync_batches are called 5 times as max 5 reties provided for function


Typo: It should be 'retries'

Fixed the typo.

karanpanchal-crest · 2021-11-17T11:28:38Z

@savan-chovatiya Please add comments to which code change corresponds to which JIRA

savan-chovatiya · 2021-11-17T11:35:24Z

tap_facebook/__init__.py

@@ -241,6 +241,7 @@ class AdCreative(Stream):
    doc: https://developers.facebook.com/docs/marketing-api/reference/adgroup/adcreatives/
    '''

+    @retry_pattern(backoff.expo, (FacebookRequestError, AttributeError), max_tries=5, factor=5)


This change is for TDL-13267: AdCreative requests that are not retrying on AttributeError

KrisPersonal

These test cases needs to be reviewed by Kyle speer. Please assign this PR to him

KrisPersonal

Add Change comments in the code

savan-chovatiya · 2021-11-18T08:18:27Z

Add Change comments in the code

Added comments for changes in code.

kspeer825

Left a suggestion

kspeer825 · 2021-11-18T13:20:29Z

tests/unittests/test_attribute_error_retry.py

+        # verify get_ad_creatives() is called 5 times as max 5 reties provided for function
+        self.assertEquals(mocked_account.get_ad_creatives.call_count, 5)
+
+    def test__call_get_ads(self, mocked_sleep):


Suggested change

def test__call_get_ads(self, mocked_sleep):

def test_call_get_ads(self, mocked_sleep):

Updated test name as per suggestion.

…r replication key field `date_start` (#172) * added format as date-time in schema file * added code coverage * added check for date format in the bookmark test * added the check for first sync messages Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

…streams and TDL-9872: replication keys are not specified as expected in discoverable metadata (#167) * added valid replication keys in catalog * modified the code * TDL-9809: Added replication keys in metadata * adde code coverage * Resolved review comments Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

#168) * TDL-7455: Added archived data integration test * TDL-7455: Updated integration test * added code coverage * Resolved review comment Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

…ve requests that are not retrying (singer-io#171) * TDL-6148: Added retry for Attribute error of sync batches * TDL-6148: Removed unused imports from unit tests * TDL-13267: Added retry for 500 error of AdCreatives * TDL-6148: Add AttributeError backoff for all sync functions * added code coverage * Resolved review comment * Resolved review comments * Added code comments * Resolved review comment * TDL-9728: Stream `ads_insights_age_gender` has unexpected datatype for replication key field `date_start` (singer-io#172) * added format as date-time in schema file * added code coverage * added check for date format in the bookmark test * added the check for first sync messages Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-9809: `forced-replication-method` missing from metadata for some streams and TDL-9872: replication keys are not specified as expected in discoverable metadata (singer-io#167) * added valid replication keys in catalog * modified the code * TDL-9809: Added replication keys in metadata * adde code coverage * Resolved review comments Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-7455: Add tap-tester test to verify replication of deleted records (singer-io#168) * TDL-7455: Added archived data integration test * TDL-7455: Updated integration test * added code coverage * Resolved review comment Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: Harsh <80324346+harshpatel4crest@users.noreply.github.com> Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com> Co-authored-by: KrisPersonal <66801357+KrisPersonal@users.noreply.github.com>

TDL-6148: Added retry for Attribute error of sync batches

075f24f

savan-chovatiya marked this pull request as draft October 11, 2021 11:51

savan-chovatiya added 2 commits October 12, 2021 12:58

TDL-6148: Removed unused imports from unit tests

0702427

TDL-13267: Added retry for 500 error of AdCreatives

63e361b

savan-chovatiya changed the title ~~TDL-6148: Catch and retry Attribute error~~ TDL-6148: Catch and retry Attribute error and TDL-13267: Fix AdCreative requests that are not retrying Oct 12, 2021

savan-chovatiya and others added 2 commits October 20, 2021 13:26

TDL-6148: Add AttributeError backoff for all sync functions

92c5e35

added code coverage

acf9a61

savan-chovatiya marked this pull request as ready for review November 12, 2021 08:44

dbshah1212 reviewed Nov 12, 2021

View reviewed changes

Resolved review comment

49ce423

savan-chovatiya requested a review from dbshah1212 November 12, 2021 13:09

karanpanchal-crest suggested changes Nov 17, 2021

View reviewed changes

dbshah1212 approved these changes Nov 17, 2021

View reviewed changes

Resolved review comments

c54cbab

savan-chovatiya commented Nov 17, 2021

View reviewed changes

savan-chovatiya requested a review from karanpanchal-crest November 17, 2021 11:38

dbshah1212 requested review from kspeer825, KrisPersonal and manand31 November 17, 2021 13:17

karanpanchal-crest approved these changes Nov 17, 2021

View reviewed changes

KrisPersonal requested changes Nov 17, 2021

View reviewed changes

Added code comments

5b6e9fe

savan-chovatiya requested a review from KrisPersonal November 18, 2021 08:18

kspeer825 approved these changes Nov 18, 2021

View reviewed changes

savan-chovatiya and others added 3 commits November 18, 2021 19:18

Resolved review comment

74b5826

savan-chovatiya and others added 3 commits November 19, 2021 14:00

TDL-7455: Add tap-tester test to verify replication of deleted records (

12bb05d

#168) * TDL-7455: Added archived data integration test * TDL-7455: Updated integration test * added code coverage * Resolved review comment Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com>

Merge branch 'master' into crest-master

b13fb22

Resolved merge conflict

8e6636b

KrisPersonal approved these changes Nov 19, 2021

View reviewed changes

Base automatically changed from crest-master to master November 19, 2021 16:19

harshpatel4_crest and others added 2 commits November 22, 2021 11:26

resolve merge conflicts

c17e173

Merge branch 'master' into TDL-6148-catch-and-retry-attribute-error

4a04b0d

KrisPersonal merged commit 427232f into master Nov 23, 2021

KrisPersonal deleted the TDL-6148-catch-and-retry-attribute-error branch November 23, 2021 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-6148: Catch and retry Attribute error and TDL-13267: Fix AdCreative requests that are not retrying #171

TDL-6148: Catch and retry Attribute error and TDL-13267: Fix AdCreative requests that are not retrying #171

savan-chovatiya commented Oct 11, 2021 •

edited

Loading

dbshah1212 Nov 12, 2021

savan-chovatiya Nov 12, 2021

karanpanchal-crest Nov 17, 2021

savan-chovatiya Nov 17, 2021

karanpanchal-crest Nov 17, 2021

savan-chovatiya Nov 17, 2021

karanpanchal-crest commented Nov 17, 2021

savan-chovatiya Nov 17, 2021

KrisPersonal left a comment

KrisPersonal left a comment

savan-chovatiya commented Nov 18, 2021

kspeer825 left a comment

kspeer825 Nov 18, 2021

savan-chovatiya Nov 18, 2021

	def test__call_get_ads(self, mocked_sleep):
	def test_call_get_ads(self, mocked_sleep):

TDL-6148: Catch and retry Attribute error and TDL-13267: Fix AdCreative requests that are not retrying #171

TDL-6148: Catch and retry Attribute error and TDL-13267: Fix AdCreative requests that are not retrying #171

Conversation

savan-chovatiya commented Oct 11, 2021 • edited Loading

Description of change

Manual QA steps

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karanpanchal-crest commented Nov 17, 2021

Choose a reason for hiding this comment

KrisPersonal left a comment

Choose a reason for hiding this comment

KrisPersonal left a comment

Choose a reason for hiding this comment

savan-chovatiya commented Nov 18, 2021

kspeer825 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savan-chovatiya commented Oct 11, 2021 •

edited

Loading