Source Facebook Marketing: implement retry logic for async reports #7904

keu · 2021-11-12T10:58:10Z

What

closes #7822

How

Introduce AsyncJob class to spawn and retry AdReport jobs

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1201354977636604

CLAassistant · 2021-11-12T10:58:15Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

keu · 2021-11-12T12:42:56Z

WIP on adding few unittests for the new class

avida · 2021-11-12T15:23:04Z

airbyte-integrations/connectors/source-facebook-marketing/setup.py

@@ -6,7 +6,7 @@
 from setuptools import find_packages, setup

 MAIN_REQUIREMENTS = [
-    "airbyte-cdk~=0.1.33",
+    "airbyte-cdk~=0.1",


Why you changed this line?

eliziario

LGTM

…b-fix-async-job

sherifnada · 2021-11-12T17:15:40Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/streams.py

@@ -280,8 +279,6 @@ class AdsInsights(FBMarketingIncrementalStream):
        "action_destination",
    ]

-    MAX_WAIT_TO_START = pendulum.duration(minutes=5)
-    MAX_WAIT_TO_FINISH = pendulum.duration(minutes=30)
    MAX_ASYNC_SLEEP = pendulum.duration(minutes=5)


Maybe we should increase this number. 5 min to wait for a job feels too low

It would be nice to have some logs for statistics to know exactly how often this happens, but from the logs that I have seen it almost never exceed this time.
Will increase this value though

sherifnada · 2021-11-12T17:18:31Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/streams.py

@@ -497,14 +444,6 @@ def _date_ranges(self, stream_state: Mapping[str, Any]) -> Iterator[dict]:
                "time_range": {"since": since.to_date_string(), "until": until.to_date_string()},
            }

-    @backoff_policy


Not blocking but it seems the new Async class should be responsible for the retry logic instead of communicating to a calling class it should be retried by raising an exception. Not blocking though

keu · 2021-11-12T18:33:53Z

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1454295087
✅ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1454295087
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        75      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  896    440    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     17    77%
	 source_facebook_marketing/async_job.py      92     58    37%
	 source_facebook_marketing/common.py         37     11    70%
	 source_facebook_marketing/source.py        112     65    42%
	 source_facebook_marketing/streams.py       233     79    66%
	 ------------------------------------------------------------
	 TOTAL                                      551    230    58%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     18    76%
	 source_facebook_marketing/async_job.py      92      1    99%
	 source_facebook_marketing/common.py         37      1    97%
	 source_facebook_marketing/source.py        112     72    36%
	 source_facebook_marketing/streams.py       233     79    66%
	 ------------------------------------------------------------
	 TOTAL                                      551    171    69%

keu · 2021-11-12T19:04:27Z

/publish connector=connectors/source-facebook-marketing

🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1454388263
✅ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1454388263

…irbytehq#7904) * added AsyncJob with retry logic Co-authored-by: Eugene Kulak <[email protected]>

eugene-kulak added 2 commits November 12, 2021 12:51

added AsyncJob with retry logic

695bccd

polishing

c608154

github-actions bot added the area/connectors Connector related issues label Nov 12, 2021

keu linked an issue Nov 12, 2021 that may be closed by this pull request

Source Facebook failed during sync with KeyError: 'id' #7822

Closed

keu requested review from sherifnada and htrueman November 12, 2021 10:58

keu temporarily deployed to more-secrets November 12, 2021 10:59 Inactive

fix issues discovered after tests

194fd87

keu temporarily deployed to more-secrets November 12, 2021 12:16 Inactive

fix issues discovered after tests

4635579

keu temporarily deployed to more-secrets November 12, 2021 12:38 Inactive

This was referenced Nov 12, 2021

Source Facebook Marketing: Fix error format that cause crash during error reporting #7823

Closed

Source Facebook failed during sync with KeyError: 'id' #7822

Closed

avida mentioned this pull request Nov 12, 2021

Fix schema transformation false warnings. #7863

Merged

write some unit tests for AsyncJob

309e314

keu requested a review from avida November 12, 2021 15:06

keu temporarily deployed to more-secrets November 12, 2021 15:07 Inactive

avida reviewed Nov 12, 2021

View reviewed changes

avida approved these changes Nov 12, 2021

View reviewed changes

eliziario approved these changes Nov 12, 2021

View reviewed changes

eugene-kulak added 2 commits November 12, 2021 18:31

update docstrings, polishing

42c545f

Merge branch 'master' of github.com:airbytehq/airbyte into keu/7822-f…

34018e5

…b-fix-async-job

github-actions bot added the area/documentation Improvements or additions to documentation label Nov 12, 2021

keu temporarily deployed to more-secrets November 12, 2021 16:38 Inactive

fix logging and polishing

8c96df0

keu temporarily deployed to more-secrets November 12, 2021 16:48 Inactive

eugene-kulak added 2 commits November 12, 2021 18:53

format

a33e142

format

3e4b3d8

keu temporarily deployed to more-secrets November 12, 2021 16:56 Inactive

sherifnada approved these changes Nov 12, 2021

View reviewed changes

bumping

cfe10c1

keu temporarily deployed to more-secrets November 12, 2021 18:28 Inactive

jrhizor temporarily deployed to more-secrets November 12, 2021 18:36 Inactive

jrhizor temporarily deployed to more-secrets November 12, 2021 19:06 Inactive

bumping

96ab6de

keu merged commit 558bc27 into master Nov 12, 2021

keu deleted the keu/7822-fb-fix-async-job branch November 12, 2021 20:08

keu temporarily deployed to more-secrets November 12, 2021 20:09 Inactive

avida mentioned this pull request Nov 29, 2021

🐛 Source Facebook Marketing: Excessive type conversion warnings #8274

Closed

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

Source Facebook Marketing: implement retry logic for async reports (a…

45fcf8e

…irbytehq#7904) * added AsyncJob with retry logic Co-authored-by: Eugene Kulak <[email protected]>

karinakuz added connectors/sources-api connectors/source/facebook-marketing labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Facebook Marketing: implement retry logic for async reports #7904

Source Facebook Marketing: implement retry logic for async reports #7904

keu commented Nov 12, 2021 •

edited

Loading

CLAassistant commented Nov 12, 2021

keu commented Nov 12, 2021

avida Nov 12, 2021

eliziario left a comment

sherifnada Nov 12, 2021

keu Nov 12, 2021

sherifnada Nov 12, 2021

keu commented Nov 12, 2021 •

edited by github-actions bot

Loading

keu commented Nov 12, 2021 •

edited by github-actions bot

Loading

Source Facebook Marketing: implement retry logic for async reports #7904

Source Facebook Marketing: implement retry logic for async reports #7904

Conversation

keu commented Nov 12, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

CLAassistant commented Nov 12, 2021

keu commented Nov 12, 2021

avida Nov 12, 2021

Choose a reason for hiding this comment

eliziario left a comment

Choose a reason for hiding this comment

sherifnada Nov 12, 2021

Choose a reason for hiding this comment

keu Nov 12, 2021

Choose a reason for hiding this comment

sherifnada Nov 12, 2021

Choose a reason for hiding this comment

keu commented Nov 12, 2021 • edited by github-actions bot Loading

keu commented Nov 12, 2021 • edited by github-actions bot Loading

keu commented Nov 12, 2021 •

edited

Loading

keu commented Nov 12, 2021 •

edited by github-actions bot

Loading

keu commented Nov 12, 2021 •

edited by github-actions bot

Loading