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

TiDB Destination: add append_dedup mode #19109

Merged
merged 9 commits into from May 23, 2023

Conversation

Daemonxiao
Copy link
Contributor

@Daemonxiao Daemonxiao commented Nov 8, 2022

What

TiDB Destination support "deduped history" mode. So add append_dedup mode in spec.json.

How

Describe the solution

Recommended reading order

  1. airbyte-integrations/connectors/destination-tidb/src/main/resources/spec.json

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • [] Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Integration

eeuVqw5uGo

@github-actions github-actions bot added the area/connectors Connector related issues label Nov 8, 2022
@Daemonxiao
Copy link
Contributor Author

/test connector=connectors/destination-tidb

@YiyangLi
Copy link
Contributor

YiyangLi commented Nov 18, 2022

/test connector=connectors/destination-tidb

🕑 connectors/destination-tidb https://github.com/airbytehq/airbyte/actions/runs/3499313396
✅ connectors/destination-tidb https://github.com/airbytehq/airbyte/actions/runs/3499313396
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                            2      0   100%
normalization/transform_catalog/reserved_keywords.py                 14      0   100%
normalization/transform_catalog/__init__.py                           2      0   100%
normalization/destination_type.py                                    14      0   100%
normalization/__init__.py                                             4      0   100%
normalization/transform_catalog/destination_name_transformer.py     166      8    95%
normalization/transform_catalog/table_name_registry.py              174     34    80%
normalization/transform_config/transform.py                         189     48    75%
normalization/transform_catalog/utils.py                             51     14    73%
normalization/transform_catalog/dbt_macro.py                         22      7    68%
normalization/transform_catalog/catalog_processor.py                147     80    46%
normalization/transform_catalog/transform.py                         61     38    38%
normalization/transform_catalog/stream_processor.py                 595    400    33%
-------------------------------------------------------------------------------------
TOTAL                                                              1441    629    56%

Build Passed

Test summary info:

All Passed

@Daemonxiao
Copy link
Contributor Author

@YiyangLi Is this PR OK to merge?

Copy link
Contributor

@YiyangLi YiyangLi left a comment

Choose a reason for hiding this comment

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

:shipit:

@YiyangLi
Copy link
Contributor

YiyangLi commented Nov 22, 2022

/publish connector=connectors/destination-tidb

🕑 Publishing the following connectors:
connectors/destination-tidb
https://github.com/airbytehq/airbyte/actions/runs/3525293788


Connector Did it publish? Were definitions generated?
connectors/destination-tidb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@YiyangLi
Copy link
Contributor

YiyangLi commented Nov 22, 2022

/publish connector=connectors/destination-tidb

🕑 Publishing the following connectors:
connectors/destination-tidb
https://github.com/airbytehq/airbyte/actions/runs/3525379516


Connector Did it publish? Were definitions generated?
connectors/destination-tidb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@marcosmarxm
Copy link
Member

Hello 👋, first thank you for this amazing contribution.

We really appreciate the effort you've made to improve the project.
We ask you patience for the code review. Last month our team was focused on Hacktoberfest event and that probably left some PR without the proper feedback. And this week, due to the Thanksgiving US Holiday, most our team is out of office with their families. Another important piece of information why code won't be merge this week is: as a safety measure the core team has decided to freeze merging code to main branch to keep the release stable. Next week we'll return to you with the proper code review and update the status of your contribution.

If you have any questions feel free to send me a message in Slack!
Thanks!

@YiyangLi
Copy link
Contributor

YiyangLi commented Nov 29, 2022

/publish connector=connectors/destination-tidb

🕑 Publishing the following connectors:
connectors/destination-tidb
https://github.com/airbytehq/airbyte/actions/runs/3577403466


Connector Did it publish? Were definitions generated?
connectors/destination-tidb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@marcosmarxm
Copy link
Member

I don't think it will work only adding the append_dedup. Probably you must change the normalization factory in the worker. Also maybe there is some work to made in normalization module.

@Daemonxiao
Copy link
Contributor Author

@marcosmarxm Thanks for your advice. 🙏
TiDB normalization module uses the dbt-tidb package. The default increment mode of dbt-tidb is delete+insert, which means it could auto-delete the duplicate row with unique keys and then insert the new row. This mode works like upsert. Besides, we have already passed the acceptance test and it has tested the append_dedup mode.

@sajarin sajarin added the bounty-L Maintainer program: claimable large bounty PR label Dec 5, 2022
@marcosmarxm
Copy link
Member

Hello 👋:skin-tone-2: and thank you for your contribution!

Airbyte has instituted a code freeze between 19 and 30 December, to make sure there are no disruptions during the holidays.
Because of this, reviewing and merging your contribution may take longer than usual.
We apologize for the delay, but we want everyone to have a quiet and happy holiday.

If you have any questions or need further clarification, please don't hesitate to ping via Slack.

@Daemonxiao
Copy link
Contributor Author

Is this pr ready to be merged? Or is there anything else I can do?

@marcosmarxm
Copy link
Member

marcosmarxm commented May 19, 2023

/test connector=connectors/destination-tidb

🕑 connectors/destination-tidb https://github.com/airbytehq/airbyte/actions/runs/5028032356
❌ connectors/destination-tidb https://github.com/airbytehq/airbyte/actions/runs/5028032356
🐛 https://gradle.com/s/g34daxfsf4f2i

Build Failed

Test summary info:

Could not find result summary

@marcosmarxm
Copy link
Member

marcosmarxm commented May 19, 2023

/test connector=connectors/destination-tidb

🕑 connectors/destination-tidb https://github.com/airbytehq/airbyte/actions/runs/5028109127
✅ connectors/destination-tidb https://github.com/airbytehq/airbyte/actions/runs/5028109127
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                            2      0   100%
normalization/transform_catalog/reserved_keywords.py                 15      0   100%
normalization/transform_catalog/__init__.py                           2      0   100%
normalization/destination_type.py                                    18      0   100%
normalization/__init__.py                                             4      0   100%
normalization/transform_catalog/destination_name_transformer.py     171     10    94%
normalization/transform_catalog/table_name_registry.py              174     34    80%
normalization/transform_config/transform.py                         195     48    75%
normalization/transform_catalog/utils.py                             51     14    73%
normalization/transform_catalog/dbt_macro.py                         22      7    68%
normalization/transform_catalog/catalog_processor.py                147     80    46%
normalization/transform_catalog/transform.py                         65     39    40%
normalization/transform_catalog/stream_processor.py                 603    407    33%
-------------------------------------------------------------------------------------
TOTAL                                                              1469    639    57%

Build Passed

Test summary info:

All Passed

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 23, 2023
@marcosmarxm marcosmarxm merged commit 1e1bdac into airbytehq:master May 23, 2023
35 of 43 checks passed
nguyenaiden pushed a commit that referenced this pull request May 25, 2023
* TiDB Destination: add append_dedup mode

* update Dockerfile

* update docs and metadata file

---------

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
marcosmarxm added a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
* TiDB Destination: add append_dedup mode

* update Dockerfile

* update docs and metadata file

---------

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation bounty bounty-L Maintainer program: claimable large bounty PR community connectors/destination/tidb pr-alpha-beta
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants