snowflake s3 copy & redshift s3 refactor #2921

jrhizor · 2021-04-16T18:18:54Z

This PR provides the ability to create streaming writes to a file and manage the issuance of copy commands for the destination.

It introduces the concept of a SwitchingDestination and has tools to reuse code for copying files to a specific staging environment (such as S3) that can be reused across destinations.

Recommended reading order:

SnowflakeDestination
airbyte-integrations/connectors/destination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/*
airbyte-integrations/connectors/destination-snowflake/src/main/java/io/airbyte/integrations/destination/snowflake/*
Redshift-related code

Remaining:

remove merge conflicts
clean up tests
add integration test
redo manual testing

cgardens

I think I followed what's going on here. I think you're adding some solid abstractions to keep the file upload approach lean for future dbs.

I'm requesting changes because there a few places where I think we can improve the clarity of how things are working. The main things are:

the responsibilities / lifecycle of the copier--in other words all of the steps that are involved
understanding the delegate concept as used in the copier.

if you plan to address all of these things in the refactor you're working on, just lmk and i can approve this one and we can figure out the clarity stuff in the next PR. i definitely want to take another look when the clarity stuff is addressed. that all being said, i think the fundamental approach here is spot on.

...ors/destination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/Copier.java

...ion-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/AbstractJdbcDestination.java

...ination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/CopierSupplier.java

...nation-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/CopyDestination.java

...estination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/s3/S3Copier.java

...ination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/CopierSupplier.java

...src/main/java/io/airbyte/integrations/destination/jdbc/copy/s3/S3CopierSupplierDelegate.java

jrhizor · 2021-04-23T20:29:42Z

/test connector=connectors/destination-snowflake

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

jrhizor · 2021-04-23T20:35:39Z

/test connector=connectors/destination-snowflake

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

cgardens

nice! much clearer now.

I had a few more comments about structure but I don't want to block. Happy to talk about them more if they are interesting.

...nation-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/CopyDestination.java

cgardens · 2021-04-23T21:16:17Z

...stination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/StreamCopier.java

+   *
+   * @return the SQL queries necessary to merge or the empty string if there was a failure
+   */
+  String copyToTmpTableAndPrepMergeToFinalTable(boolean hasFailed) throws Exception;


i'm still not in love this method. would it make sense to split it up? looking at the impl it kinda looks like that's how you think about it anway.

closeWriter / closerConsumer

copyDataToTmpTable

generateMergeStatement

dunno. i might be off course here.

From a developer perspective I really dont know that I should do with the function.

Also because it has so many responsibilities that can fail, all the error handling will have to be done in every implementation instead of in the consumer of that interface.

.../src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeCopyS3Destination.java

cgardens · 2021-04-23T22:02:29Z

...ake/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeS3StreamCopier.java

+import io.airbyte.protocol.models.DestinationSyncMode;
+import java.sql.SQLException;
+
+public class SnowflakeS3StreamCopier extends S3StreamCopier {


maybe purely a style thing, but what about just doing a CopyFromStorageToTable iface and use that for this part as opposed to extending S3StreamCopier? Similar to JdbcStreamingQueryConfiguration. just a thought.

michel-tricot · 2021-04-23T23:26:02Z

airbyte-integrations/connectors/destination-jdbc/build.gradle

@@ -9,6 +9,10 @@ application {
 }

 dependencies {
+    implementation 'org.apache.commons:commons-lang3:3.11'
+    implementation 'org.apache.commons:commons-csv:1.4'
+    implementation 'com.github.alexmojaki:s3-stream-upload:2.2.2'


How reliable is that library?

It's a pretty lightweight library and is regularly updated: https://github.com/alexmojaki/s3-stream-upload
However, there aren't many public usages: https://github.com/search?q=alexmojaki+%22s3-stream-upload%22&type=code

I only used this since it was already part of @davinchia's Redshift implementation. I imagine if we do run into problems with it we can rip it out easily, so I'm not planning on changing it to something internal preemptively.

...stination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/CopyConsumer.java

michel-tricot · 2021-04-23T23:41:52Z

...stination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/StreamCopier.java

+   *
+   * @return the SQL queries necessary to merge or the empty string if there was a failure
+   */
+  String copyToTmpTableAndPrepMergeToFinalTable(boolean hasFailed) throws Exception;


From a developer perspective I really dont know that I should do with the function.

Also because it has so many responsibilities that can fail, all the error handling will have to be done in every implementation instead of in the consumer of that interface.

...n-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/SwitchingDestination.java

...ke/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationType.java

jrhizor · 2021-04-26T15:07:09Z

I changed the interface to move all of the control flow to CopyConsumer.closeAsOneTransaction:

      for (var copier : streamCopiers) {
        copier.closeStagingUploader(hasFailed);

        if (!hasFailed) {
          copier.createTemporaryTable();
          copier.copyStagingFileToTemporaryTable();
          copier.createDestinationSchema();
          var destTableName = copier.createDestinationTable();
          var mergeQuery = copier.generateMergeStatement(destTableName);
          mergeCopiersToFinalTableQuery.append(mergeQuery);
        }
      }

Which I think reads a lot better.

jrhizor · 2021-04-26T15:07:43Z

/test connector=connectors/destination-snowflake

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

jrhizor · 2021-04-26T15:07:52Z

/test connector=connectors/destination-redshift

🕑 connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/786020239
❌ connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/786020239

jrhizor · 2021-04-26T15:36:15Z

/test connector=connectors/destination-snowflake

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

jrhizor · 2021-04-26T15:36:53Z

/test connector=connectors/destination-redshift

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

jrhizor · 2021-04-26T15:51:34Z

/publish connector=connectors/destination-snowflake

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

jrhizor · 2021-04-26T15:53:57Z

/publish connector=connectors/destination-redshift

🕑 connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/786158453
❌ connectors/destination-redshift https://github.com/airbytehq/airbyte/actions/runs/786158453

jrhizor · 2021-04-26T15:54:09Z

/publish connector=connectors/destination-snowflake

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

jrhizor · 2021-04-26T16:07:12Z

/publish connector=connectors/destination-redshift

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

jrhizor · 2021-04-26T16:07:20Z

/publish connector=connectors/destination-snowflake

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

jrhizor added 3 commits April 16, 2021 11:10

snowflake s3 copy

d36d2d8

refactor (some tests still need updating)

716fe5f

revert accidentally removing files

5be85d6

jrhizor changed the title ~~snowflake s3 copy~~ snowflake s3 copy & redshift s3 refactor Apr 20, 2021

jrhizor marked this pull request as ready for review April 20, 2021 15:39

auto-assign bot requested review from ChristopheDuong and davinchia April 20, 2021 15:39

jrhizor added 2 commits April 20, 2021 08:44

re-add purge

a304049

Merge branch 'master' into jrhizor/snowflake-s3-copy

8c0cdbb

cgardens self-requested a review April 20, 2021 20:04

cgardens requested changes Apr 20, 2021

View reviewed changes

jrhizor added 12 commits April 21, 2021 08:59

use baseconnector

819596c

getconnection logs error

8e70167

use generic configs for copiers/suppliers/consumers

8d42367

use stream copier terminology

9f6ebae

remove weird delegate generics

98bf3fa

some test changes

1ed38fe

remove non-ci test that doesn't have a good equivalent atm

761ebab

misc

e4fe676

Merge branch 'master' into jrhizor/snowflake-s3-copy

a580e24

finally fixed

ddae224

tests and fix

f88f65b

add credentials

f1b1591

fix redshift build

75eb4c3

cgardens self-requested a review April 23, 2021 21:05

cgardens approved these changes Apr 23, 2021

View reviewed changes

michel-tricot reviewed Apr 24, 2021

View reviewed changes

respond to comments

10b2b20

fix check

ce9178a

bump versions for redshift and snowflake

099d931

fix creds

d4cb978

jrhizor merged commit a3b4444 into master Apr 26, 2021

jrhizor deleted the jrhizor/snowflake-s3-copy branch April 26, 2021 16:41

karinakuz added connectors/destinations-warehouse connectors/destination/snowflake labels Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snowflake s3 copy & redshift s3 refactor #2921

snowflake s3 copy & redshift s3 refactor #2921

jrhizor commented Apr 16, 2021 •

edited

Loading

cgardens left a comment

jrhizor commented Apr 23, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 23, 2021 •

edited by github-actions bot

Loading

cgardens left a comment

cgardens Apr 23, 2021

michel-tricot Apr 23, 2021

cgardens Apr 23, 2021

michel-tricot Apr 23, 2021

jrhizor Apr 26, 2021

michel-tricot Apr 23, 2021

jrhizor commented Apr 26, 2021

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

snowflake s3 copy & redshift s3 refactor #2921

snowflake s3 copy & redshift s3 refactor #2921

Conversation

jrhizor commented Apr 16, 2021 • edited Loading

cgardens left a comment

Choose a reason for hiding this comment

jrhizor commented Apr 23, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 23, 2021 • edited by github-actions bot Loading

cgardens left a comment

Choose a reason for hiding this comment

cgardens Apr 23, 2021

Choose a reason for hiding this comment

michel-tricot Apr 23, 2021

Choose a reason for hiding this comment

cgardens Apr 23, 2021

Choose a reason for hiding this comment

michel-tricot Apr 23, 2021

Choose a reason for hiding this comment

jrhizor Apr 26, 2021

Choose a reason for hiding this comment

michel-tricot Apr 23, 2021

Choose a reason for hiding this comment

jrhizor commented Apr 26, 2021

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 26, 2021 • edited by github-actions bot Loading

jrhizor commented Apr 16, 2021 •

edited

Loading

jrhizor commented Apr 23, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 23, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading

jrhizor commented Apr 26, 2021 •

edited by github-actions bot

Loading