-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 Amazon Ads: fix reports stream records primary keys #21677
Source Amazon Ads: fix reports stream records primary keys #21677
Conversation
/test connector=connectors/source-amazon-ads
Build PassedTest summary info:
|
...ons/connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/products_report.py
Outdated
Show resolved
Hide resolved
...ions/connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/report_streams.py
Show resolved
Hide resolved
...ons/connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/products_report.py
Outdated
Show resolved
Hide resolved
...ions/connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/report_streams.py
Outdated
Show resolved
Hide resolved
Can you lay out the possible incompatible changes? |
It's on description. Not sure what else can I add more, unless concrete question |
What is the preparation needed - is it on our end or on the customer's end? If it's on the customer's end we should figure out how many people are impacted by this so that TCS can coordinate
It's unclear to me who needs to discuss/agree on what changes - does this only refer to the |
...ions/connectors/source-amazon-ads/source_amazon_ads/streams/report_streams/report_streams.py
Outdated
Show resolved
Hide resolved
Hey guys, Roman is unavailable for a couple of days so I will pick up this issue. Serhii and I discussed this problem and the suggested solution and decided that it needs to be thinked over one more time and probably reimplemented. Suggested solution fixes existing problem but most probably introduces a new one. |
Thank you for the update @davydov-d! |
/test connector=connectors/source-amazon-ads
Build FailedTest summary info:
|
@erohmensing PR updated, please review |
/test connector=connectors/source-amazon-ads
Build PassedTest summary info:
|
@davydov-d would you please rebase? There are lots of unrelated changes in the diff. Otherwise I believe it looks good |
ed6fd30
to
b8e71d0
Compare
…eam-record-primary-keys
b8e71d0
to
4e67027
Compare
…eam-record-primary-keys
@erohmensing Sorry, I overlooked it, must have been pre-commit hooks. Removed non related changes. |
/test connector=connectors/source-amazon-ads
Build PassedTest summary info:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to update TCS with any changes customers will have to make and get their OK before merging (they may want to schedule it if it is still breaking) - thanks for adopting this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Reminder there is a breaking change so we'll need to coordinate with TCS to release this to Cloud
/publish connector=connectors/source-amazon-ads
if you have connectors that successfully published but failed definition generation, follow step 4 here |
Airbyte Code Coverage
|
* master: Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164)
* master: (86 commits) Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164) Update connector-acceptance-tests-reference.md (#22370) Update the default security groups for the EC2 runner (#22347) Trace refresh schema operations (#22326) Remove manual docker upgrades from workflows (#22344) Update CODEOWNERS for connector acceptance tests to connector ops (#22341) 🐛 source: airtable - handle singleSelect types (#22311) Source tiktok: chunk advertiser IDs (#22309) 🪟 🧪 E2E Tests for auto-detect schema changes (#20682) ...
* master: (24 commits) Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164) Update connector-acceptance-tests-reference.md (#22370) Update the default security groups for the EC2 runner (#22347) Trace refresh schema operations (#22326) Remove manual docker upgrades from workflows (#22344) Update CODEOWNERS for connector acceptance tests to connector ops (#22341) 🐛 source: airtable - handle singleSelect types (#22311) Source tiktok: chunk advertiser IDs (#22309) 🪟 🧪 E2E Tests for auto-detect schema changes (#20682) ...
What
Because of non-unique values are defined as a composite primary key of the report streams, we are missing a bunch of records per each sync if dedup+history strategy chosen. This PR contains a fix but also adds some possibly incompatible changes that needs to be discussed. At this point I splitted
asins
report to two reportsasins_keywords
andasins_targets
. We need this separation for everything to be consistent but maybe it require some preparation before delivery to cloud.