Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(source-google-ads): get_customers implementation #46893

Merged
merged 4 commits into from
Oct 15, 2024
Merged

fix(source-google-ads): get_customers implementation #46893

merged 4 commits into from
Oct 15, 2024

Conversation

darynaishchenko
Copy link
Collaborator

What

oc: https://github.com/airbytehq/oncall/issues/6650
Discover OOM on platform

How

When user have a big amount of customers in their account it could lead to OOM issue.

Updated get_customers method to use from_accounts_by_id when customer_ids were provided otherwise use from_accounts to get all account.

Logic for skipping duplicates were moved to the CustomerModel to filter customers only once.

Review guide

  1. airbyte-integrations/connectors/source-google-ads/source_google_ads/models.py
  2. airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@darynaishchenko darynaishchenko self-assigned this Oct 14, 2024
@vercel Vercel
Copy link

vercel bot commented Oct 14, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Oct 14, 2024 2:04pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/google-ads labels Oct 14, 2024
@darynaishchenko
Copy link
Collaborator Author

regression tests:
with customers in the config: succeeded
without customers in the config: succeeded

@darynaishchenko
Copy link
Collaborator Author

darynaishchenko commented Oct 14, 2024 •
edited by github-actions bot
Loading

/approve-regression-tests

Check job output.

✅ Approving regression tests

@darynaishchenko darynaishchenko requested a review from a team October 14, 2024 15:25
aldogonzalez8
aldogonzalez8 approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@aldogonzalez8 aldogonzalez8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVED

@darynaishchenko darynaishchenko merged commit 6404ba3 into master Oct 15, 2024
39 checks passed
@darynaishchenko darynaishchenko deleted the daryna/source-google-ads/discovery-oom branch October 15, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bazarnov bazarnov bazarnov approved these changes

@aldogonzalez8 aldogonzalez8 aldogonzalez8 approved these changes

Assignees

@darynaishchenko darynaishchenko

Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/google-ads
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@darynaishchenko @bazarnov @aldogonzalez8 @octavia-squidington-iii