Plumb together Copy Redshift #2606

davinchia · 2021-03-24T15:40:18Z

What

Describe what the change is solving
Follow up to #2547.

Plumb everything together to allow a user to, via configuration, pick either the less efficient and easier to set up INSERT strategy, or the more efficient and slightly more set up COPY strategy.

Tests gives us the following results:

number of rows	insert	copy
1k	16s	20s
10k	24s	25s
100k	1m 28s	44s
500k	5m 30s	1m 30s
1 Million	10m 20s	3m 4s

Effectiveness of COPY is obvious with larger and larger number of rows for a rough 3 - 4 x improvement. Logs show we are now effectively read constrained.

How

Modify the JSON spec to take additional S3 configuration. The RedshiftDestination is split into the InsertDestination and the CopyDestination. The RedshiftDestination picks the CopyDestination if this S3 configuration exists, falling back to the InsertDestination otherwise.

The original RedshiftDestination is now the RedshiftInsertDestination class - all that previous functionality is identical.

Pre-merge Checklist

Run integration tests
Publish Docker images

Recommended reading order

json.spec - to look at spec changes.
RedshiftDestination - to understand how the underlying destination is picked.
RedshiftCopyDestination - to understand how the new copy strategy functions.

davinchia · 2021-03-24T15:41:15Z

...redshift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftDestination.java

  }

  @Override
-  public JsonNode toJdbcConfig(JsonNode redshiftConfig) {
-    final String schema = Optional.ofNullable(redshiftConfig.get("schema")).map(JsonNode::asText).orElse("public");
+  public DestinationConsumer<AirbyteMessage> write(JsonNode config, ConfiguredAirbyteCatalog catalog) throws Exception {


@cgardens just a skeleton - does this seem like a reasonable approach to support both destinations?

yeah. this seems reasonable to me for keeping a clear separation.

should we just dependency inject the 2 different destinations to make testing easier? then this outer RedshiftDestination is essentially just a decorator.

public RedshiftDestination() { this(new RedshiftInsertDestination(), new RedshiftCopyDestination()); } public RedshiftDestination(Destination insertDestination, Destination copyDestination) { ...etc }

…shift

davinchia · 2021-03-25T16:54:07Z

@cgardens want to take another pass? No test yet, and still some possible code clean up - but this is working!

davinchia · 2021-03-25T17:00:27Z

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

@@ -45,6 +45,34 @@
        "description": "Whether or not to normalize the data in the destination. See <a href=\"https://docs.airbyte.io/architecture/basic-normalization\">basic normalization</a> for more details.",
        "title": "Basic Normalization",
        "examples": [true, false]
+      },
+      "access_key_id": {


was running into some strange json schema stuff so put that on pause now - will relook tomorrow

…arameter extraction.

davinchia · 2021-03-26T17:39:51Z

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

@@ -45,6 +45,54 @@
        "description": "Whether or not to normalize the data in the destination. See <a href=\"https://docs.airbyte.io/architecture/basic-normalization\">basic normalization</a> for more details.",
        "title": "Basic Normalization",
        "examples": [true, false]
+      },
+      "s3_bucket_name": {


I spent 2 hours or so running into some frontend weirdness with spec.json rendering. I decided to cut my losses and go with this for now. What we lose is 1 additional authentication method (using ARN roles) + some JSON niceties. Not a big deal some Singer doesn't support the arn role too and I've compensated for this with a more descriptive JSON schema object. I feel it's more valuable to release what we have now and bolt the rest of this on later. I'm also ready to stop working on Redshift :P Does that sound sane?

Any bad input is caught by the https://github.com/airbytehq/airbyte/pull/2606/files#diff-719915fe360d60f49387522687fbde9a7ae807bc837c319edb242b977359e417R202 for the time being. Will write a ticket describing what I was trying to do and what did/did not work.

This is how this UI looks.

davinchia · 2021-03-26T17:42:44Z

PTAL @cgardens @sherifnada. This is 90% there. The only thing left is 1/2 test cases on the RedshiftDestination class making the relevant changes in the integration tests. Will be great if you can review by Monday (no worries otherwise!)

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

sherifnada · 2021-03-26T22:26:40Z

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

+      "secret_access_key": {
+        "type": "string",
+        "description": "The corresponding secret to the above access key id.",
+        "title": "S3 Access Key"


Suggested change

"title": "S3 Access Key"

"title": "S3 Access Key",

"airbyte_secret": true

...hift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftCopyDestination.java

sherifnada · 2021-03-26T23:26:16Z

...redshift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftDestination.java

-    super(DRIVER_CLASS, new RedshiftSQLNameTransformer(), new RedshiftSqlOperations());
+  @Override
+  public DestinationConsumer<AirbyteMessage> write(JsonNode config, ConfiguredAirbyteCatalog catalog) throws Exception {
+    if (hasCopyConfigs(config)) {


might be good to DRY this if block into a method

I played around with this and the other approach I came up with was DRY but also somewhat complicated and I prefer this form. I'm going to merge this in first to close out the ticket. Happy to iterate on this with you outside. Do you have a particular structure in mind?

...hift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftCopyDestination.java

sherifnada · 2021-03-26T23:36:24Z

...hift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftCopyDestination.java

+      this.secretAccessKey = config.get("secret_access_key").asText();
+    }
+
+    public static boolean isPresent(JsonNode config) {


why not move this check to the constructor and throw an IllegalArgumentException or some other checked exception? this way we don't have to remember to call isPresent

this function does validation and is also used by the RedshiftDestination to decide which underlying strategy to use.

moving this to the constructor works, however it feels strange to be switching on an exception instead of an explicit check. I prefer this form. again, will merge in for now and iterate on your outside if you feel strongly.

davinchia · 2021-03-29T10:52:32Z

/test connector=connectors/desintation-redshift

🕑 connectors/desintation-redshift https://github.com/airbytehq/airbyte/actions/runs/697580153
❌ connectors/desintation-redshift https://github.com/airbytehq/airbyte/actions/runs/697580153

davinchia · 2021-03-29T10:53:11Z

/test connector=connectors/destination-redshift

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

davinchia · 2021-03-29T12:49:22Z

/test connector=connectors/destination-redshift

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

…shift

davinchia · 2021-03-29T13:12:59Z

/publish connector=connectors/destination-redshift

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

Spike on how to switch between Destinations.

a91049c

davinchia commented Mar 24, 2021

View reviewed changes

Davin Chia added 3 commits March 25, 2021 14:09

Merge remote-tracking branch 'origin' into davinchia/plug-old-new-red…

e42142c

…shift

Checkpoint: initial spec and get check connection working.

dfeb4b0

Plumb everything together.

ecf021d

davinchia commented Mar 25, 2021

View reviewed changes

Davin Chia added 3 commits March 26, 2021 15:42

Minimise logging. Get to a stable spec.json.

6c14cd9

Remove unnecessary logging. Refactor to centralise S3 Configuration p…

d0d7edb

…arameter extraction.

Better spec json description.

f950c5a

davinchia requested a review from jrhizor March 26, 2021 12:56

davinchia marked this pull request as ready for review March 26, 2021 17:30

auto-assign bot requested review from cgardens and michel-tricot March 26, 2021 17:30

Correct formatting.

e9f3141

davinchia requested review from sherifnada and removed request for jrhizor and michel-tricot March 26, 2021 17:37

davinchia commented Mar 26, 2021

View reviewed changes

jrhizor reviewed Mar 26, 2021

View reviewed changes

airbyte-integrations/connectors/destination-redshift/src/main/resources/spec.json Show resolved Hide resolved

sherifnada approved these changes Mar 26, 2021

View reviewed changes

Davin Chia added 4 commits March 29, 2021 15:58

Respond to PR feedback.

a1573d5

Remove unneeded test.

e0e4ad7

Add more test cases.

d991806

remove accidentally added private.

934c090

Split integration tests into Insert and Copy.

c289c41

Davin Chia added 3 commits March 29, 2021 21:03

Add comments.

a789661

Merge remote-tracking branch 'origin' into davinchia/plug-old-new-red…

179d510

…shift

Update docker tag. Format comments.

d29b70b

davinchia merged commit 947e24b into master Mar 29, 2021

davinchia deleted the davinchia/plug-old-new-redshift branch March 29, 2021 14:21

davinchia mentioned this pull request Mar 30, 2021

Improve loading into Redshift Destination. #2569

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plumb together Copy Redshift #2606

Plumb together Copy Redshift #2606

davinchia commented Mar 24, 2021 •

edited

Loading

davinchia Mar 24, 2021

cgardens Mar 24, 2021

cgardens Mar 24, 2021 •

edited

Loading

davinchia commented Mar 25, 2021

davinchia Mar 25, 2021

davinchia Mar 26, 2021

davinchia Mar 26, 2021

davinchia commented Mar 26, 2021

sherifnada Mar 26, 2021

davinchia Mar 29, 2021

sherifnada Mar 26, 2021

davinchia Mar 29, 2021 •

edited

Loading

sherifnada Mar 26, 2021

davinchia Mar 29, 2021

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

	"title": "S3 Access Key"
	"title": "S3 Access Key",
	"airbyte_secret": true

Plumb together Copy Redshift #2606

Plumb together Copy Redshift #2606

Conversation

davinchia commented Mar 24, 2021 • edited Loading

What

How

Pre-merge Checklist

Recommended reading order

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

davinchia commented Mar 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia commented Mar 29, 2021 • edited by github-actions bot Loading

davinchia commented Mar 29, 2021 • edited by github-actions bot Loading

davinchia commented Mar 29, 2021 • edited by github-actions bot Loading

davinchia commented Mar 29, 2021 • edited by github-actions bot Loading

davinchia commented Mar 24, 2021 •

edited

Loading

cgardens Mar 24, 2021 •

edited

Loading

davinchia Mar 29, 2021 •

edited

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading

davinchia commented Mar 29, 2021 •

edited by github-actions bot

Loading