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 & Destination sync modes are required #2500

Merged
merged 10 commits into from
Mar 19, 2021

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Mar 17, 2021

What

Following discussion on default sync modes: #2460 (comment)
we've agreed to make the sync mode fields required but provide a migration script that makes sure it is always defined for older standard syncs.

How

Make fields required in ConfiguredAirbyteStream and provide migration.

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/models/airbyte_protocol.py
  2. airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_18_0.java
  3. the rest

@@ -2125,7 +2125,7 @@ components:
- append
- overwrite
#- upsert_dedup # TODO chris: SCD Type 1 can be implemented later
- append_dedup # SCD Type 2
- append_dedup # SCD Type 1 & 2
Copy link
Contributor

Choose a reason for hiding this comment

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

why is 1 and 2 now? should the comment above be removed then?

Copy link
Contributor

Choose a reason for hiding this comment

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

is there not a field in this file that needs to set sync mode to required? or is it already set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

append_dedup produces two tables:

  • history table (SCD type 2)
  • dedup table (SCD type 1)
    So I just made the comment truthier

upsert_dedup without the history table would use less space (since no SCD type 2 would be stored)

@ChristopheDuong ChristopheDuong force-pushed the chris/required-sync-modes branch from 3d4c95a to 4b5c22a Compare March 18, 2021 14:39
@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-bigquery

🕑 connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/667561982
✅ connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/667561982

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-csv

🕑 connectors/destination-csv https://github.com/airbytehq/airbyte/actions/runs/667561962
✅ connectors/destination-csv https://github.com/airbytehq/airbyte/actions/runs/667561962

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-local-json

🕑 connectors/destination-local-json https://github.com/airbytehq/airbyte/actions/runs/667562059
✅ connectors/destination-local-json https://github.com/airbytehq/airbyte/actions/runs/667562059

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-meilisearch

🕑 connectors/destination-meilisearch https://github.com/airbytehq/airbyte/actions/runs/667562156
✅ connectors/destination-meilisearch https://github.com/airbytehq/airbyte/actions/runs/667562156

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-postgres

🕑 connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/667562206
✅ connectors/destination-postgres https://github.com/airbytehq/airbyte/actions/runs/667562206

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-redshift

🕑 connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/667563609
✅ connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/667563609

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/667562257
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/667562257

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/667602644
❌ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/667602644
🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/667602644
❌ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/667602644

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/source-mssql

🕑 connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/667602926
✅ connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/667602926

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/publish connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/668121244
✅ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/668121244

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/test connector=source-mysql

🕑 source-mysql https://github.com/airbytehq/airbyte/actions/runs/668179759
❌ source-mysql https://github.com/airbytehq/airbyte/actions/runs/668179759

@ChristopheDuong
Copy link
Contributor Author

ChristopheDuong commented Mar 19, 2021

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/668179975
✅ source-postgres https://github.com/airbytehq/airbyte/actions/runs/668179975

@ChristopheDuong ChristopheDuong merged commit 231efae into chris/dedup-destination Mar 19, 2021
@ChristopheDuong ChristopheDuong deleted the chris/required-sync-modes branch March 19, 2021 15:02
ChristopheDuong added a commit that referenced this pull request Mar 26, 2021
* Handle destination sync mode in destinations

* Source & Destination sync modes are required (#2500)

* Provide Migration script making sure it is always defined for previous sync configs
@sherifnada
Copy link
Contributor

sherifnada commented Mar 29, 2021

@ChristopheDuong I'm also not sure python connectors would work like this if we didn't push a new update to base-python and updated all connectors to begin using it.

also doesn't this break all existing integration tests? I think the only reason it hasn't is because we didn't publish a new version of the base python image and change connectors to rely on it.

@sherifnada
Copy link
Contributor

I guess not because additionalProperties is set to true on ConfiguredAirbyteStream, so it can handle reading configured catalogs... was this intentional?

@ChristopheDuong
Copy link
Contributor Author

I guess not because additionalProperties is set to true on ConfiguredAirbyteStream, so it can handle reading configured catalogs... was this intentional?

Yes, we want to make changes to the protocol (like in this PR by adding a new field for destination sync mode) without breaking all connectors (they should ignore the new field if they don't support it):

Related PR was probably: #2238

@ChristopheDuong
Copy link
Contributor Author

Actually, since the field is required for ConfiguredAirbyteStream it might break things as you describe?

if we left it as a new optional field, then all would be good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants