☝🏼Destinations supports destination sync mode #2460

ChristopheDuong · 2021-03-15T15:46:38Z

What

Describe what the change is solving
Closes #2409

Pre-merge Checklist

Run integration tests
Publish Docker images

cgardens

looks good!

i think we need to make sure we write integrations tests for normalization so that we all be in agreement on what exactly the behavior is we expect. this shouldn't be part of the PR, but i'm just pointing it out so it doesn't get forgotten.

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/AcceptanceTests.java

cgardens · 2021-03-15T20:27:27Z

airbyte-workers/src/main/java/io/airbyte/workers/DefaultSyncWorker.java

+    LOGGER.info("configured destination sync modes: {}", syncInput.getCatalog().getStreams()
+        .stream()
+        .collect(Collectors.toMap(s -> s.getStream().getName(),
+            s -> s.getDestinationSyncMode() != null ? s.getDestinationSyncMode() : DestinationSyncMode.APPEND)));


is this assumption safe? it seems like if syncMode is full_refresh and destinationSyncMode is null then we'll end up with destinationSyncMode being set to append. In the case i think the intended configuration is full refresh, but we'll end up with incremental. it seems like for now if destinationSyncMode is empty you need to set using information from sync mode to figure out what it should be? i could be misunderstanding.

maybe you could write a test that shows the mapping of the possible configurations to what you expect them to be so that we can all be on the same page?

Discussed offline and agreed on moving forward with 2 changes:

we'll be removing the default values of sync modes and raising exceptions if they are not defined...

I will be providing a Migration Script to make sure destination sync modes are defined when upgrading versions and are keeping the same behavior as before the upgrade

Since they become required I'll start another PR for that change instead.
In the meantime, I can merge this one as it is

PR is here: #2500

...rs/destination-csv/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java

...-json/src/main/java/io/airbyte/integrations/destination/local_json/LocalJsonDestination.java

ChristopheDuong · 2021-03-16T12:56:27Z

airbyte-scheduler/src/main/java/io/airbyte/scheduler/persistence/DefaultJobCreator.java

@@ -124,7 +125,7 @@ public long createGetSpecJob(String integrationImage) throws IOException {
  }

  // Strategy:
-  // 1. Set all streams to full refresh.
+  // 1. Set all streams to full refresh - overwrite.
  // 2. Create a job where the source emits no records.
  // 3. Run a sync from the empty source to the destination. This will overwrite all data for each
  // stream in the destination.


Instead of an empty source, could we force a full refresh from source instead?
Talking with @johnlafleur, we feel it would be friendlier to the user and so I created this ticket: #2484

ChristopheDuong · 2021-03-16T13:49:56Z

/publish connector=connectors/destination-postgres

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

ChristopheDuong · 2021-03-16T14:30:41Z

/publish connector=connectors/destination-bigquery

🕑 connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/657891412
❌ connectors/destination-bigquery https://github.com/airbytehq/airbyte/actions/runs/657891412

ChristopheDuong · 2021-03-16T14:30:46Z

/publish connector=connectors/destination-snowflake

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

ChristopheDuong · 2021-03-16T14:30:52Z

/publish connector=connectors/destination-redshift

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

ChristopheDuong · 2021-03-16T14:30:59Z

/publish connector=connectors/destination-csv

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

ChristopheDuong · 2021-03-16T14:31:13Z

/publish connector=connectors/destination-local-json

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

ChristopheDuong · 2021-03-16T14:31:22Z

/publish connector=connectors/destination-meilisearch

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

…connector

ChristopheDuong · 2021-03-16T15:44:22Z

/publish connector=connectors/destination-bigquery

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

* Make sync modes required * Provide Migration script making sure it is always defined for previous sync configs

ChristopheDuong · 2021-03-19T15:06:50Z

Since this PR now introduces destination connectors that require destination sync modes to be defined, I will wait until the PR for the front-end (#2371) is ready before merging this one to master.

Otherwise, newly created sync will be failing since the UI won't be setting/defining destination sync modes yet

ChristopheDuong added 3 commits March 15, 2021 11:04

Handle destination sync mode in destinations

ceaf2ce

Destination behavior follow destination sync mode

7349353

acceptance tests destiantion sync mode

ae2a15a

auto-assign bot requested review from cgardens and sherifnada March 15, 2021 15:46

ChristopheDuong changed the title ~~Chris/dedup destination~~ Destinations supports destination sync mode Mar 15, 2021

Bumpversions of connectors

d24071c

cgardens approved these changes Mar 15, 2021

View reviewed changes

michel-tricot reviewed Mar 16, 2021

View reviewed changes

...rs/destination-csv/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java Outdated Show resolved Hide resolved

...-json/src/main/java/io/airbyte/integrations/destination/local_json/LocalJsonDestination.java Outdated Show resolved Hide resolved

Changes from code review

826bdc6

ChristopheDuong commented Mar 16, 2021

View reviewed changes

Handle null cases for destination sync mode for bigquery destination …

579dabb

…connector

sherifnada approved these changes Mar 16, 2021

View reviewed changes

ChristopheDuong mentioned this pull request Mar 17, 2021

Source & Destination sync modes are required #2500

Merged

2 tasks

ChristopheDuong added 2 commits March 17, 2021 19:00

Merge remote-tracking branch 'origin/master' into dedup-destination

c909912

Merge remote-tracking branch 'origin/master' into dedup-destination

976271a

ChristopheDuong mentioned this pull request Mar 18, 2021

Increase time-out of tests? #2515

Merged

ChristopheDuong added 2 commits March 19, 2021 11:25

Merge remote-tracking branch 'origin/master' into dedup-destination

35fcdd6

Source & Destination sync modes are required (#2500)

231efae

* Make sync modes required * Provide Migration script making sure it is always defined for previous sync configs

ChristopheDuong mentioned this pull request Mar 19, 2021

Destinations support destination sync modes #2409

Closed

ChristopheDuong changed the title ~~Destinations supports destination sync mode~~ ☝🏼Destinations supports destination sync mode Mar 19, 2021

ChristopheDuong mentioned this pull request Mar 24, 2021

Add previously missing cursor types to JDBC utils. #2600

Merged

2 tasks

ChristopheDuong added 2 commits March 24, 2021 17:34

Merge remote-tracking branch 'origin/master' into dedup-destination

de2e09b

Merge remote-tracking branch 'origin/master' into dedup-destination

ba0cb0d

ChristopheDuong merged commit 8a29584 into master Mar 26, 2021

ChristopheDuong deleted the chris/dedup-destination branch March 26, 2021 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

☝🏼Destinations supports destination sync mode #2460

☝🏼Destinations supports destination sync mode #2460

ChristopheDuong commented Mar 15, 2021 •

edited

Loading

cgardens left a comment

cgardens Mar 15, 2021

ChristopheDuong Mar 16, 2021

ChristopheDuong Mar 16, 2021

ChristopheDuong Mar 17, 2021

ChristopheDuong Mar 16, 2021 •

edited

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 19, 2021 •

edited

Loading

☝🏼Destinations supports destination sync mode #2460

☝🏼Destinations supports destination sync mode #2460

Conversation

ChristopheDuong commented Mar 15, 2021 • edited Loading

What

Pre-merge Checklist

cgardens left a comment

Choose a reason for hiding this comment

cgardens Mar 15, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 16, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 16, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 17, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 16, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 19, 2021 • edited Loading

ChristopheDuong commented Mar 15, 2021 •

edited

Loading

ChristopheDuong Mar 16, 2021 •

edited

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 16, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 19, 2021 •

edited

Loading