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.

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

Source Facebook Marketing: fix start_date selection behaviour #36025

Merged
merged 24 commits into from Mar 20, 2024

Conversation

artem1205
Copy link
Collaborator

What

Resolve https://github.com/airbytehq/oncall/issues/4495

How

fix logic of start_date to sync from;
considering lookback window do not sync data prior start_date

Recommended reading order

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

🚨 User Impact 🚨

no breaking changes

Pre-merge Actions

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Unit & integration tests added

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.

Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Copy link

vercel bot commented Mar 13, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Mar 20, 2024 1:02pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/facebook-marketing labels Mar 13, 2024
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
@artem1205 artem1205 marked this pull request as ready for review March 13, 2024 19:32
@octavia-squidington-iv octavia-squidington-iv requested a review from a team March 13, 2024 19:33
@@ -314,6 +314,10 @@ def _get_start_date(self) -> Mapping[str, pendulum.Date]:
start_date = min(start_date, refresh_date)
else:
start_date = self._start_date

if start_date < self._start_date:
logger.warning(f"Ignore provided state and start sync from start_date ({self._start_date}).")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we have the message like this?

logger.warning(f"Starting the sync from: ({self._start_date}). The provided state: {%} was ignored.")

Not sure about the state mention in this context, but it seems like one of [start_date, self._start_date, oldest_date] ?

@octavia-squidington-iv octavia-squidington-iv requested a review from a team March 13, 2024 19:50
@erohmensing
Copy link
Contributor

@artem1205 I want to check - when we are moving up the start date, are the end dates for the slices adjusted accordingly? Looking into this and I want to make sure it is addressed (since the start and end date that were overlapping were both before the user's prescribed start date, I'm hoping that this was caused by the same root cause but feel free to redirect me if you think otherwise)

@artem1205
Copy link
Collaborator Author

@erohmensing , Yep, it should be covered. see unit tests starting from test_stream_slices_with_state_close_to_now

…cebook-OC-4495-2

# Conflicts:
#	airbyte-integrations/connectors/source-facebook-marketing/metadata.yaml
#	airbyte-integrations/connectors/source-facebook-marketing/pyproject.toml
#	docs/integrations/sources/facebook-marketing.md
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
@artem1205 artem1205 merged commit ccab316 into master Mar 20, 2024
30 checks passed
@artem1205 artem1205 deleted the artem1205/source-facebook-OC-4495-2 branch March 20, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/facebook-marketing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants