Source Facebook marketing: Fix duplicating records during insights lookback period #13047
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sergey Chvalyuk <[email protected]>
github-actions
bot
added
area/connectors
|
/test connector=connectors/source-facebook-marketing
|
Codecov Report
@@ Coverage Diff @@
## master #13047 +/- ##
=========================================
Coverage ? 91.72%
=========================================
Files ? 11
Lines ? 870
Branches ? 0
=========================================
Hits ? 798
Misses ? 72
Partials ? 0 Continue to review full report at Codecov.
|
Signed-off-by: Sergey Chvalyuk <[email protected]>
…cessed_in_lookback_period Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
|
/test connector=connectors/source-facebook-marketing
|
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
grubberr
changed the title
Signed-off-by: Sergey Chvalyuk <[email protected]>
|
/test connector=connectors/source-facebook-marketing
|
evantahler
reviewed
May 22, 2022
Comment on lines
113
to
114
| if date_start and pendulum.parse(record["updated_time"]).date() <= date_start: | ||
| continue |
There was a problem hiding this comment.
Nice - this seems to be the main logical change
Comment on lines
163
to
164
| today = pendulum.today(tz="UTC").date() | ||
| end_date = min(self._end_date, today) |
There was a problem hiding this comment.
Perhaps since we are going to prevent looking at "today" again once that date has already been looked up, the earliest day we look at should be "yesterday" (e.g. today - 1). This way, we won't try to look up data for a partial day without being able to update it.
There was a problem hiding this comment.
I have changed logic to sync only for yesterday
Signed-off-by: Sergey Chvalyuk <[email protected]>
|
/test connector=connectors/source-facebook-marketing
|
evantahler
approved these changes
May 23, 2022
|
/publish connector=connectors/source-facebook-marketing
|
|
/publish connector=connectors/source-facebook-marketing
|
jscottpolevault
pushed a commit
to jscottpolevault/airbyte
that referenced
this pull request
Jun 1, 2022
…okback period (airbytehq#13047) Signed-off-by: Sergey Chvalyuk <[email protected]>
|
@grubberr @evantahler there is something I don't fully understand, in this change, the connector is still downloading the data for the last 28 days but only commits the records with date later than the state. Is that right? Wouldn't make more sense to call the FB API with that filter to avoid unnecessary calls? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Sergey Chvalyuk [email protected]
What
Connector during incremental stream constantly re-read all records for
insights_loopback_period(default 28 days) producing duplicate records.Improve logic to re-read only record which were updated.
Recommended reading order
x.javay.python🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.