Refactor Snowflake internal Staging as a base class for other staging classes #10865

ChristopheDuong · 2022-03-04T20:58:13Z

What

In preparation for other PRs, this one is isolating some changes to make other PRs easier to follow.
Such as: #10866

How

Refactor the Snowflake Internal Staging destination to re-use it as a base for other staging options.

🚨 User Impact 🚨

Nothing is changing. We don't even need to re-publish a snowflake connector.

…es in jdbc destinations

…rnal-staging

ChristopheDuong · 2022-03-08T13:41:32Z

/test connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/1951719336
❌ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/1951719336
🐛

edgao

added a couple comments but I think I mostly understand

...n-jdbc/src/main/java/io/airbyte/integrations/destination/staging/StagingConsumerFactory.java

...destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3DestinationConfig.java

tuliren · 2022-03-09T16:17:10Z

...base-java/src/main/java/io/airbyte/integrations/destination/NamingConventionTransformer.java

@@ -45,4 +47,8 @@
  @Deprecated
  String getTmpTableName(String name);

+  String getStageName(String schemaName, String tableName);
+
+  String getStagingPath(String connectionId, String schemaName, String tableName, DateTime writeDatetime);


These methods do not look necessary for all destination. Should they be put in this interface?

Correct! thank you, I'll move them to stagingOperations

...ses/base-java/src/main/java/io/airbyte/integrations/destination/StandardNameTransformer.java

…rnal-staging

CLAassistant · 2022-03-10T12:56:28Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ ChristopheDuong
❌ Chris Duong

Chris Duong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov · 2022-03-10T13:06:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1bf9470). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #10865   +/-   ##
=========================================
  Coverage          ?   78.06%           
=========================================
  Files             ?       12           
  Lines             ?     1304           
  Branches          ?        0           
=========================================
  Hits              ?     1018           
  Misses            ?      286           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bf9470...2a923e7. Read the comment docs.

ChristopheDuong · 2022-03-10T16:29:43Z

/test connector=connectors/destination-snowflake

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

Name                                                                                                                            Stmts   Miss  Cover
---------------------------------------------------------------------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                                                                                          2      0   100%
normalization/transform_catalog/reserved_keywords.py                                                                               13      0   100%
normalization/transform_catalog/__init__.py                                                                                         2      0   100%
normalization/destination_type.py                                                                                                  13      0   100%
normalization/__init__.py                                                                                                           4      0   100%
/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/models/airbyte_protocol.py     124      0   100%
/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/models/__init__.py               1      0   100%
/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/__init__.py                      2      0   100%
normalization/transform_catalog/destination_name_transformer.py                                                                   155      8    95%
normalization/transform_catalog/table_name_registry.py                                                                            174     34    80%
normalization/transform_config/transform.py                                                                                       150     30    80%
normalization/transform_catalog/utils.py                                                                                           33      7    79%
normalization/transform_catalog/catalog_processor.py                                                                              143     77    46%
normalization/transform_catalog/transform.py                                                                                       45     26    42%
normalization/transform_catalog/stream_processor.py                                                                               524    337    36%
---------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                                            1385    519    63%

edgao

one last comment, feel free to merge if I'm misunderstanding how the class works

...n-jdbc/src/main/java/io/airbyte/integrations/destination/staging/StagingConsumerFactory.java

… classes (#10865) * Refactor Snowflake internal Staging as model to share staging abilities in jdbc destinations

Refactor Snowflake internal Staging as model to share staging abiliti…

e88d6a3

…es in jdbc destinations

github-actions bot added the area/connectors Connector related issues label Mar 4, 2022

ChristopheDuong temporarily deployed to more-secrets March 4, 2022 20:58 Inactive

ChristopheDuong temporarily deployed to more-secrets March 4, 2022 20:59 Inactive

ChristopheDuong added 3 commits March 8, 2022 13:13

Merge remote-tracking branch 'origin/master' into chris/refactor-inte…

9f53dad

…rnal-staging

Clean up refactoring of snowflake internal staging

66180ab

format code

4fe437c

ChristopheDuong temporarily deployed to more-secrets March 8, 2022 13:15 Inactive

Move out of copy package to staging

fdceb2f

ChristopheDuong temporarily deployed to more-secrets March 8, 2022 13:24 Inactive

ChristopheDuong temporarily deployed to more-secrets March 8, 2022 13:25 Inactive

ChristopheDuong requested review from tuliren and edgao March 8, 2022 18:37

ChristopheDuong marked this pull request as ready for review March 8, 2022 18:38

edgao reviewed Mar 8, 2022

View reviewed changes

tuliren reviewed Mar 9, 2022

View reviewed changes

ChristopheDuong changed the title ~~Refactor Snowflake internal Staging~~ Refactor Snowflake internal Staging as a base class for other staging classes Mar 10, 2022

Merge remote-tracking branch 'origin/master' into chris/refactor-inte…

e177de0

…rnal-staging

ChristopheDuong temporarily deployed to more-secrets March 10, 2022 12:58 Inactive

Refactor staging name methods in staging operations

aa5eba1

ChristopheDuong force-pushed the chris/refactor-internal-staging branch from 651c7f7 to aa5eba1 Compare March 10, 2022 13:00

ChristopheDuong temporarily deployed to more-secrets March 10, 2022 13:02 Inactive

Fix test

f719c93

github-actions bot added the area/platform issues related to the platform label Mar 10, 2022

github-actions bot added the area/worker Related to worker label Mar 10, 2022

ChristopheDuong temporarily deployed to more-secrets March 10, 2022 16:25 Inactive

ChristopheDuong requested a review from edgao March 10, 2022 16:48

edgao approved these changes Mar 10, 2022

View reviewed changes

...n-jdbc/src/main/java/io/airbyte/integrations/destination/staging/StagingConsumerFactory.java Outdated Show resolved Hide resolved

Remove redundant stage creation

45c12de

ChristopheDuong temporarily deployed to more-secrets March 11, 2022 09:41 Inactive

ChristopheDuong temporarily deployed to more-secrets March 11, 2022 09:42 Inactive

ChristopheDuong mentioned this pull request Mar 11, 2022

🎉 Change destination-snowflake buffering when staging to reduce/stabilize memory/thread consumption #10866

Merged

format code

8aa02b7

ChristopheDuong temporarily deployed to more-secrets March 11, 2022 12:15 Inactive

format code

2a923e7

ChristopheDuong temporarily deployed to more-secrets March 11, 2022 13:24 Inactive

ChristopheDuong temporarily deployed to more-secrets March 11, 2022 13:25 Inactive

ChristopheDuong temporarily deployed to more-secrets March 11, 2022 13:43 Inactive

ChristopheDuong merged commit 744e0d5 into master Mar 11, 2022

ChristopheDuong deleted the chris/refactor-internal-staging branch March 11, 2022 14:29

octavia-squidington-iii mentioned this pull request Mar 11, 2022

Bump Airbyte version from 0.35.51-alpha to 0.35.52-alpha #11075

Merged

terencecho pushed a commit that referenced this pull request Mar 14, 2022

Refactor Snowflake internal Staging as a base class for other staging…

366d1a0

… classes (#10865) * Refactor Snowflake internal Staging as model to share staging abilities in jdbc destinations

bleonard added the team/databases label Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Snowflake internal Staging as a base class for other staging classes #10865

Refactor Snowflake internal Staging as a base class for other staging classes #10865

ChristopheDuong commented Mar 4, 2022 •

edited

Loading

ChristopheDuong commented Mar 8, 2022 •

edited by github-actions bot

Loading

edgao left a comment

tuliren Mar 9, 2022

ChristopheDuong Mar 10, 2022 •

edited

Loading

CLAassistant commented Mar 10, 2022

codecov bot commented Mar 10, 2022 •

edited

Loading

ChristopheDuong commented Mar 10, 2022 •

edited by github-actions bot

Loading

edgao left a comment

Refactor Snowflake internal Staging as a base class for other staging classes #10865

Refactor Snowflake internal Staging as a base class for other staging classes #10865

Conversation

ChristopheDuong commented Mar 4, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

ChristopheDuong commented Mar 8, 2022 • edited by github-actions bot Loading

edgao left a comment

Choose a reason for hiding this comment

tuliren Mar 9, 2022

Choose a reason for hiding this comment

ChristopheDuong Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

CLAassistant commented Mar 10, 2022

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

ChristopheDuong commented Mar 10, 2022 • edited by github-actions bot Loading

edgao left a comment

Choose a reason for hiding this comment

ChristopheDuong commented Mar 4, 2022 •

edited

Loading

ChristopheDuong commented Mar 8, 2022 •

edited by github-actions bot

Loading

ChristopheDuong Mar 10, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

ChristopheDuong commented Mar 10, 2022 •

edited by github-actions bot

Loading