🎉 source-facebook-marketing: allow configuration of page_size #12171

mdibaiee · 2022-04-20T10:14:01Z

What

Solves Facebook can hit limits in request size with the current page_size=100 #12168

How

Allows configuration of page_size

🚨 User Impact 🚨

No, the default stays 100.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

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
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

sherifnada

Is there any way that the connector can automatically adjust page size based on this error? This option will be fairly confusing for users otherwise as were exposing an implementation detail

mdibaiee · 2022-04-20T21:31:08Z

@sherifnada 100 is a magic number here, I don't think there is any explicit reason to use 100 as a fixed number (let me know if there is).

One solution is to try to be smart about this error, means we need to have a sort of retry logic where the page_size is automatically reduced when this error is hit. This seems to be your suggestion. Imagine a scenario where this happens often (e.g. every request), do we send 2 requests (or more, depending on the adjustment logic) every time? I'm not sure if it's possible to globally change the page_size since each call may be a separate read call on the connector, and I don't know if there is any "state" to be shared between read calls.
Another way is to just reduce this default from 100 to something smaller. I suppose this means more requests need to be sent in general but again, 100 is a magic number and one could argue for a larger number reducing number of requests.
The other way, which is this PR, is to let the user tweak the number to find something suitable for their data, if need be. Most of the time the default will probably be fine.
... any other ideas?

mdibaiee · 2022-04-21T16:54:07Z

To put the issue into perspective, I tried using this branch in our system and we ended up having to reduce the page_size to 25 to get it working without an error, that's 1/4 of the current page_size.

mdibaiee · 2022-04-21T17:31:28Z

@sherifnada I'm wondering if there is any kind of configuration that can be used when interacting with the connectors through Docker, but is not user-facing?

sherifnada · 2022-04-21T22:11:03Z

@mdibaiee this sparked some healthy internal discussion -- I think the right go forward here is to merge your contribution (made some comments on the copy). We'll create a couple of follow-up issues to:

Make this an "advanced" configuration per Allow annotating and separating advanced options for each connector config in the UI #3681
Auto-determine this setting inside the connector

sherifnada

@mdibaiee could you copy-paste the checklist for updating a connector from here into the PR description and fill it out for this contribution? Also if you can sign the CLA we can move forward on this!

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

sherifnada · 2022-04-21T22:21:56Z

Also @mdibaiee I'm curious -- how do you plan on setting this value in Estuary? Are you expecting it to be an operational fix e.g: if it starts failing for a user, an operator would go and reset this for the user? Or are you exposing it to the end user directly and expecting them to fix on their own?

mdibaiee · 2022-04-22T08:49:32Z

@sherifnada At the moment, a mix of both: we operate some deployments ourselves manually at the moment, but once our UI launches, this will be exposed to users along with all other parameters.

We also understand and share the concern here that this option is not something we want the user to care about, but at this point the work required to hide this away seems more than we can commit to at the moment.

mdibaiee · 2022-04-22T09:15:00Z

I ran the unit tests and they pass. The integration tests however, do not pass with my personal credentials, but it appears to me that it's because there are certain expectations about the data in the account (e.g. certain number of deleted stuff). The logs for integration tests also include my credentials so I didn't paste it.

sherifnada · 2022-04-22T16:33:08Z

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/2208916158
❌ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/2208916158
🐛 https://gradle.com/s/mqp2cdadx2jbm
Python short test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_match_expected[inputs0] - AssertionError:...
=================== 1 failed, 21 passed in 572.01s (0:09:32) ===================

mdibaiee · 2022-04-22T17:04:26Z

I'll send a fix for that integration test failure, I know what it is.

harshithmullapudi · 2022-04-25T14:52:19Z

/test connector=source-facebook-marketing

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

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              285    106    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  886    259    71%
Name                                                        Stmts   Miss  Cover
-------------------------------------------------------------------------------
source_facebook_marketing/streams/__init__.py                   2      0   100%
source_facebook_marketing/spec.py                              34      0   100%
source_facebook_marketing/__init__.py                           2      0   100%
source_facebook_marketing/api.py                               96     12    88%
source_facebook_marketing/streams/base_streams.py             127     27    79%
source_facebook_marketing/streams/common.py                    41     13    68%
source_facebook_marketing/streams/streams.py                   97     32    67%
source_facebook_marketing/source.py                            39     16    59%
source_facebook_marketing/streams/base_insight_streams.py     129     54    58%
source_facebook_marketing/streams/async_job.py                204    128    37%
source_facebook_marketing/streams/async_job_manager.py         74     56    24%
-------------------------------------------------------------------------------
TOTAL                                                         845    338    60%
Name                                                        Stmts   Miss  Cover
-------------------------------------------------------------------------------
source_facebook_marketing/streams/async_job.py                204      0   100%
source_facebook_marketing/streams/__init__.py                   2      0   100%
source_facebook_marketing/spec.py                              34      0   100%
source_facebook_marketing/__init__.py                           2      0   100%
source_facebook_marketing/streams/common.py                    41      1    98%
source_facebook_marketing/source.py                            39      1    97%
source_facebook_marketing/streams/async_job_manager.py         74      3    96%
source_facebook_marketing/api.py                               96      9    91%
source_facebook_marketing/streams/base_insight_streams.py     129     13    90%
source_facebook_marketing/streams/streams.py                   97     22    77%
source_facebook_marketing/streams/base_streams.py             127     30    76%
-------------------------------------------------------------------------------
TOTAL                                                         845     79    91%

marcosmarxm · 2022-04-27T19:38:09Z

@sherifnada can you check @mdibaiee changes and see if we can merge it?

harshithmullapudi · 2022-05-05T05:52:42Z

/test connector=source-facebook-marketing

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

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  80     17    79%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              285    106    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  896    259    71%
Name                                                        Stmts   Miss  Cover
-------------------------------------------------------------------------------
source_facebook_marketing/streams/__init__.py                   2      0   100%
source_facebook_marketing/spec.py                              34      0   100%
source_facebook_marketing/__init__.py                           2      0   100%
source_facebook_marketing/api.py                               96     12    88%
source_facebook_marketing/streams/base_streams.py             127     27    79%
source_facebook_marketing/streams/common.py                    41     13    68%
source_facebook_marketing/streams/streams.py                   97     32    67%
source_facebook_marketing/source.py                            39     16    59%
source_facebook_marketing/streams/base_insight_streams.py     129     54    58%
source_facebook_marketing/streams/async_job.py                210    134    36%
source_facebook_marketing/streams/async_job_manager.py         78     60    23%
-------------------------------------------------------------------------------
TOTAL                                                         855    348    59%
Name                                                        Stmts   Miss  Cover
-------------------------------------------------------------------------------
source_facebook_marketing/streams/async_job.py                210      0   100%
source_facebook_marketing/streams/__init__.py                   2      0   100%
source_facebook_marketing/spec.py                              34      0   100%
source_facebook_marketing/__init__.py                           2      0   100%
source_facebook_marketing/streams/common.py                    41      1    98%
source_facebook_marketing/source.py                            39      1    97%
source_facebook_marketing/streams/async_job_manager.py         78      3    96%
source_facebook_marketing/api.py                               96      9    91%
source_facebook_marketing/streams/base_insight_streams.py     129     13    90%
source_facebook_marketing/streams/streams.py                   97     22    77%
source_facebook_marketing/streams/base_streams.py             127     30    76%
-------------------------------------------------------------------------------
TOTAL                                                         855     79    91%

harshithmullapudi · 2022-05-05T07:36:08Z

/publish connector=connectors/source-facebook-marketing auto-bump-version=false

🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/2274307406
🚀 Successfully published connectors/source-facebook-marketing
✅ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/2274307406

* source-facebook-marketing: allow configuration of page_size * source-facebook-marketing: fix integration test for page_size conf * fix: bin are added * chore: update seed file Co-authored-by: Harshith Mullapudi <harshithmullapudi@gmail.com>

github-actions bot added the area/connectors Connector related issues label Apr 20, 2022

octavia-squidington-iii added the community label Apr 20, 2022

mdibaiee mentioned this pull request Apr 20, 2022

Facebook can hit limits in request size with the current page_size=100 #12168

Closed

mdibaiee force-pushed the 12168/configure-page-size branch from 667b693 to cda4567 Compare April 20, 2022 10:48

mdibaiee mentioned this pull request Apr 20, 2022

Get user running and fully functional estuary/flow#454

Closed

marcosmarxm requested review from sherifnada and lazebnyi April 20, 2022 16:00

sherifnada suggested changes Apr 20, 2022

View reviewed changes

sherifnada reviewed Apr 21, 2022

View reviewed changes

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/spec.py Outdated Show resolved Hide resolved

mdibaiee changed the title ~~source-facebook-marketing: allow configuration of page_size~~ 🎉 source-facebook-marketing: allow configuration of page_size Apr 22, 2022

mdibaiee force-pushed the 12168/configure-page-size branch from e301e40 to 57a1c4b Compare April 22, 2022 09:14

github-actions bot added the area/documentation Improvements or additions to documentation label Apr 22, 2022

marcosmarxm assigned harshithmullapudi Apr 27, 2022

sherifnada approved these changes Apr 28, 2022

View reviewed changes

mdibaiee added 2 commits May 5, 2022 11:12

source-facebook-marketing: allow configuration of page_size

644e59b

source-facebook-marketing: fix integration test for page_size conf

a3ceeb2

harshithmullapudi force-pushed the 12168/configure-page-size branch from ccd1a17 to a3ceeb2 Compare May 5, 2022 05:43

github-actions bot added area/api Related to the api area/platform issues related to the platform labels May 5, 2022

github-actions bot added area/protocol area/scheduler labels May 5, 2022

fix: bin are added

f4b671d

github-actions bot removed area/protocol area/platform issues related to the platform area/api Related to the api area/scheduler labels May 5, 2022

chore: update seed file

0814c5d

harshithmullapudi merged commit 0601605 into airbytehq:master May 5, 2022

mdibaiee deleted the 12168/configure-page-size branch May 5, 2022 13:37

octavia-squidington-iii mentioned this pull request May 5, 2022

Bump Airbyte version from 0.36.9-alpha to 0.36.10-alpha #12626

Merged

lazebnyi removed their request for review May 30, 2022 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 source-facebook-marketing: allow configuration of page_size #12171

🎉 source-facebook-marketing: allow configuration of page_size #12171

mdibaiee commented Apr 20, 2022 •

edited

Loading

sherifnada left a comment

mdibaiee commented Apr 20, 2022 •

edited

Loading

mdibaiee commented Apr 21, 2022

mdibaiee commented Apr 21, 2022

sherifnada commented Apr 21, 2022

sherifnada left a comment

sherifnada commented Apr 21, 2022

mdibaiee commented Apr 22, 2022

mdibaiee commented Apr 22, 2022

sherifnada commented Apr 22, 2022 •

edited by github-actions bot

Loading

mdibaiee commented Apr 22, 2022

harshithmullapudi commented Apr 25, 2022 •

edited by github-actions bot

Loading

marcosmarxm commented Apr 27, 2022

harshithmullapudi commented May 5, 2022 •

edited by github-actions bot

Loading

harshithmullapudi commented May 5, 2022 •

edited by github-actions bot

Loading

🎉 source-facebook-marketing: allow configuration of page_size #12171

🎉 source-facebook-marketing: allow configuration of page_size #12171

Conversation

mdibaiee commented Apr 20, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Tests

sherifnada left a comment

Choose a reason for hiding this comment

mdibaiee commented Apr 20, 2022 • edited Loading

mdibaiee commented Apr 21, 2022

mdibaiee commented Apr 21, 2022

sherifnada commented Apr 21, 2022

sherifnada left a comment

Choose a reason for hiding this comment

sherifnada commented Apr 21, 2022

mdibaiee commented Apr 22, 2022

mdibaiee commented Apr 22, 2022

sherifnada commented Apr 22, 2022 • edited by github-actions bot Loading

mdibaiee commented Apr 22, 2022

harshithmullapudi commented Apr 25, 2022 • edited by github-actions bot Loading

marcosmarxm commented Apr 27, 2022

harshithmullapudi commented May 5, 2022 • edited by github-actions bot Loading

harshithmullapudi commented May 5, 2022 • edited by github-actions bot Loading

mdibaiee commented Apr 20, 2022 •

edited

Loading

mdibaiee commented Apr 20, 2022 •

edited

Loading

sherifnada commented Apr 22, 2022 •

edited by github-actions bot

Loading

harshithmullapudi commented Apr 25, 2022 •

edited by github-actions bot

Loading

harshithmullapudi commented May 5, 2022 •

edited by github-actions bot

Loading

harshithmullapudi commented May 5, 2022 •

edited by github-actions bot

Loading