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

Fix JDBC Test. #2821

Merged
merged 2 commits into from
Apr 9, 2021
Merged

Fix JDBC Test. #2821

merged 2 commits into from
Apr 9, 2021

Conversation

davinchia
Copy link
Contributor

What

Fix JDBC test.

How

  • Remove supported_destination_sync_modes from the jdbc spec.json.
  • Add condition to test - only test dedup if a destination implements basic normalization.

Pre-merge Checklist

  • Run integration tests

Recommended reading order

Two small files.

@davinchia
Copy link
Contributor Author

davinchia commented Apr 9, 2021

@ChristopheDuong stumbled upon this while implementing my namespace change. I think this is right based on my understanding - need normalisation in order to dedup so the append dedup test shouldn't run for connectors that don't support basic normalization. does this make sense?

@davinchia
Copy link
Contributor Author

davinchia commented Apr 9, 2021

/test connector=destination-jdbc

🕑 destination-jdbc https://github.com/airbytehq/airbyte/actions/runs/732016972
✅ destination-jdbc https://github.com/airbytehq/airbyte/actions/runs/732016972

@davinchia davinchia mentioned this pull request Apr 9, 2021
14 tasks
@ChristopheDuong
Copy link
Contributor

Today the way the append_dedup is implemented is only in the normalization code so yes you are right they are both required.

But we could have a connector that decides to implement append_dedup (in meilisearch for example by upserting into indexes instead of appending?) without ever implementing normalization...

So maybe the better fix here is to remove the append_dedup option from the JDBC destination supported_destination_sync_modes, you can leave the 'append' and 'overwrite' ones though.

But still, maybe we should keep your addition for testing support of the basic normalization because of the way the test is implemented which are very tied to normalization implementation of dedup.

@davinchia
Copy link
Contributor Author

davinchia commented Apr 9, 2021

/test connector=destination-jdbc

🕑 destination-jdbc https://github.com/airbytehq/airbyte/actions/runs/732578400
✅ destination-jdbc https://github.com/airbytehq/airbyte/actions/runs/732578400

@davinchia
Copy link
Contributor Author

Made the changes @ChristopheDuong ! How does this look?

@davinchia davinchia merged commit 2d0abb8 into master Apr 9, 2021
@davinchia davinchia deleted the davinchia/fix-jdbc-test branch April 9, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants