Modify RedshiftConnectionManager to extend from SQLConnectionManager, migrate from psycopg2 to redshift python connector #251

sathiish-kumar · 2022-12-19T23:10:36Z

define a _get_connect_method method to leverage Redshift python connector to retrieve the connect method

resolves #219

Description

This is a draft PR to solicit opinions on the changes that would help in the eventual migration to Redshift python connector from psycopg2 connector. Due to its draft nature, the PR doesn't have any tests.

Extend RedshiftConnectionManager from SQLConnectionManager
Implement mandatory methods from the abstract SQLConnectionManager
Define _get_connect_method method to provide low-level Redshift Python Connector based connection methods for establishing Connections

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

… define a _get_connect_method method to leverage Redshift python connector to retrieve the connect method

cla-bot · 2022-12-19T23:10:40Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @sathiish-kumar

…ct_method

cla-bot · 2022-12-30T00:54:05Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @sathiish-kumar

cla-bot · 2023-01-03T23:20:04Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @sathiish-kumar

dbt/adapters/redshift/connections.py

…h due to in-built support in Redshift Python Connector

cla-bot · 2023-01-09T19:48:21Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @sathiish-kumar

colin-rogers-dbt · 2023-01-13T19:09:34Z

dbt/adapters/redshift/connections.py

-    keepalives_idle: int = 4
+    iam_duration_seconds: Optional[
+        int
+    ] = 900  # TODO:Not supported in redshift python connector, should we remove this?


if it doesn't support these options is it because it's no longer needed or because the functionality isn't supported?

This is obfuscated from the users of redshift python connector, it's managed by redshift connector whenever we want to use IAM profile as the mode of authn. In essence, no longer needed.

dbt/adapters/redshift/connections.py

sathiish-kumar · 2023-01-13T19:52:45Z

dbt/adapters/redshift/connections.py

+
+    @classmethod
+    def get_response(cls, cursor: redshift_connector.Cursor) -> AdapterResponse:
+        rows = cursor.rowcount


@colin-rogers-dbt to follow-up on the removal of cursor.statusmessage in get_response

sathiish-kumar · 2023-01-13T19:54:47Z

dbt/adapters/redshift/connections.py

+    connect_timeout: int = 10
+    role: Optional[str] = None
+    sslmode: Optional[str] = None
+    sslcert: Optional[


@colin-rogers-dbt to follow-up on sslcert, sslrootcert and sslkey usage and if it's really needed to be kept.

cc: @jtcohen6 &@Fleid
Looking around I don't think this is being used (and I don't think it even makes sense for redshift anyways) so we can probably remove. AFAIK I don't think having extra values in the profile.yml will break anything but we should probably add a warning that it's no longer utilized?

Not being used to my knowledge. Confirmed that users will not see an error if they add extra unused / unrecognized values into profiles.yml. I think a deprecation warning is reasonable, given that these were previously documented.

colin-rogers-dbt

two nits but otherwise this should be good to go

setup.py

dbt/adapters/redshift/connections.py

…role, application_name

colin-rogers-dbt · 2023-02-10T01:59:58Z

waiting on some other large changes to merge this

Fleid · 2023-02-15T21:45:17Z

\o/

… migrate from psycopg2 to redshift python connector (dbt-labs#251) * Change RedshiftConnectionManager to extend from SQLConnectionManager, define a _get_connect_method method to leverage Redshift python connector to retrieve the connect method * Add/fix unit tests, create RedshiftConnectMethodFactory to vend connect_method * Fix _connection_keys to mimic PostgresConnectionManager * Remove unneeded functions for tmp_cluster_creds and env_var creds auth due to in-built support in Redshift Python Connector * Resolve some TODOs * Fix references to old exceptions, add changelog * Fix errors with functional tests by overriding add_query & execute and modifying multi statement execution * Attempt to fix integration tests by adding `valid_incremental_strategies` in impl.py * Fix unit tests * Attempt to fix integration tests * add unit tests for execute * add unit tests for add_query * make get_connection_method work with serverless * add unit tests for serverless iam connections * add redshift connector version, remove sslmode, connection time out, role, application_name * change redshift_connector version --------- Co-authored-by: jiezhec <[email protected]> Co-authored-by: colin-rogers-dbt <[email protected]>

Change RedshiftConnectionManager to extend from SQLConnectionManager,…

ab4b269

… define a _get_connect_method method to leverage Redshift python connector to retrieve the connect method

sathiish-kumar marked this pull request as draft December 20, 2022 19:12

Add/fix unit tests, create RedshiftConnectMethodFactory to vend conne…

ff9fdfd

…ct_method

Fix _connection_keys to mimic PostgresConnectionManager

fbd5731

Brooke-white reviewed Jan 4, 2023

View reviewed changes

dbt/adapters/redshift/connections.py Outdated Show resolved Hide resolved

dbt/adapters/redshift/connections.py Outdated Show resolved Hide resolved

Remove unneeded functions for tmp_cluster_creds and env_var creds aut…

4f98546

…h due to in-built support in Redshift Python Connector

colin-rogers-dbt reviewed Jan 13, 2023

View reviewed changes

Fleid mentioned this pull request Jan 13, 2023

[CT-1817] [PR Review] Change RedshiftConnectionManager to extend from SQLConnectionManager (#251) #267

Closed

sathiish-kumar commented Jan 13, 2023

View reviewed changes

dbt/adapters/redshift/connections.py Outdated Show resolved Hide resolved

sathiish-kumar commented Jan 13, 2023

View reviewed changes

dbt/adapters/redshift/connections.py Outdated Show resolved Hide resolved

sathiish-kumar commented Jan 13, 2023

View reviewed changes

colin-rogers-dbt marked this pull request as ready for review January 13, 2023 19:59

colin-rogers-dbt added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Jan 13, 2023

sathiish-kumar added 2 commits January 17, 2023 16:03

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

f724708

Resolve some TODOs

5319b90

cla-bot bot added the cla:yes label Jan 18, 2023

sathiish-kumar changed the title ~~[DRAFT] Change RedshiftConnectionManager to extend from SQLConnectionManager~~ Modify RedshiftConnectionManager to extend from SQLConnectionManager, migrate from psycopg2 to redshift python connector Jan 18, 2023

Fix references to old exceptions, add changelog

16666db

colin-rogers-dbt added the ok_to_test label Jan 18, 2023

colin-rogers-dbt closed this Jan 18, 2023

colin-rogers-dbt reopened this Jan 18, 2023

colin-rogers-dbt added ok_to_test test all ok to test and removed ok_to_test labels Jan 18, 2023

cla-bot bot added the cla:yes label Jan 27, 2023

sathiish-kumar and others added 6 commits January 30, 2023 07:05

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

4bb97ab

add unit tests for add_query

cfad7ff

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

b1c8e00

make get_connection_method work with serverless

12eb89b

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

9a319ac

add unit tests for serverless iam connections

d3113ca

jiezhen-chen force-pushed the migrate_psycopg2_to_rshift_connector branch from 1dc3ebe to d3113ca Compare February 3, 2023 16:03

colin-rogers-dbt reviewed Feb 8, 2023

View reviewed changes

setup.py Outdated Show resolved Hide resolved

dbt/adapters/redshift/connections.py Outdated Show resolved Hide resolved

colin-rogers-dbt and others added 3 commits February 8, 2023 15:19

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

f5743b4

add redshift connector version, remove sslmode, connection time out, …

880941c

…role, application_name

change redshift_connector version

8527832

colin-rogers-dbt approved these changes Feb 9, 2023

View reviewed changes

colin-rogers-dbt added 2 commits February 10, 2023 09:11

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

405a702

Merge branch 'main' into migrate_psycopg2_to_rshift_connector

0a5899e

colin-rogers-dbt merged commit 2cc47bb into dbt-labs:main Feb 15, 2023

This was referenced Feb 16, 2023

Support redshift serverless #139

Closed

[CT-837] Support IAM authentication for Redshift Serverless #138

Closed

dbeatty10 mentioned this pull request Mar 1, 2023

[CT-2243] [Regression] dbt seed doesn't work on certain cases (large row counts) #347

Closed

2 tasks

dbeatty10 mentioned this pull request Apr 28, 2023

[ADAP-500] [Bug] dbt-redshift 1.5 does not work with non-standard redshift hostnames #419

Closed

2 tasks

pdebelak mentioned this pull request May 19, 2023

[ADAP-564] [Regression] unpack_from requires a buffer of at least 5 bytes for unpacking 5 bytes at offset 0 (actual buffer size is 0) #456

Closed

2 tasks

dataders mentioned this pull request Jun 1, 2023

support multi-statement .execute() calls aws/amazon-redshift-python-driver#162

Open

dbeatty10 mentioned this pull request Jun 7, 2023

[ADAP-610] [Spike] Investigate if region is absolutely required #484

Closed

dataders mentioned this pull request Jun 9, 2023

Overwrite incremental strategies #318

Closed

6 tasks

dataders mentioned this pull request Jul 13, 2023

[ADAP-701] [Regression] query commenting is not possible from 1.5.0 onwards #536

Closed

2 tasks

dbeatty10 mentioned this pull request Jul 13, 2023

add query commenting back #537

Merged

4 tasks

dataders mentioned this pull request Apr 25, 2024

[Bug] status and rowcount not reported beginning with version 1.5.0 #785

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify RedshiftConnectionManager to extend from SQLConnectionManager, migrate from psycopg2 to redshift python connector #251

Modify RedshiftConnectionManager to extend from SQLConnectionManager, migrate from psycopg2 to redshift python connector #251

sathiish-kumar commented Dec 19, 2022

cla-bot bot commented Dec 19, 2022

cla-bot bot commented Dec 30, 2022

cla-bot bot commented Jan 3, 2023

cla-bot bot commented Jan 9, 2023

colin-rogers-dbt Jan 13, 2023

sathiish-kumar Jan 13, 2023

sathiish-kumar Jan 13, 2023

sathiish-kumar Jan 13, 2023

colin-rogers-dbt Jan 18, 2023

jtcohen6 Jan 18, 2023

colin-rogers-dbt left a comment

colin-rogers-dbt commented Feb 10, 2023

Fleid commented Feb 15, 2023

Modify RedshiftConnectionManager to extend from SQLConnectionManager, migrate from psycopg2 to redshift python connector #251

Modify RedshiftConnectionManager to extend from SQLConnectionManager, migrate from psycopg2 to redshift python connector #251

Conversation

sathiish-kumar commented Dec 19, 2022

Description

Checklist

cla-bot bot commented Dec 19, 2022

cla-bot bot commented Dec 30, 2022

cla-bot bot commented Jan 3, 2023

cla-bot bot commented Jan 9, 2023

colin-rogers-dbt Jan 13, 2023

Choose a reason for hiding this comment

sathiish-kumar Jan 13, 2023

Choose a reason for hiding this comment

sathiish-kumar Jan 13, 2023

Choose a reason for hiding this comment

sathiish-kumar Jan 13, 2023

Choose a reason for hiding this comment

colin-rogers-dbt Jan 18, 2023

Choose a reason for hiding this comment

jtcohen6 Jan 18, 2023

Choose a reason for hiding this comment

colin-rogers-dbt left a comment

Choose a reason for hiding this comment

colin-rogers-dbt commented Feb 10, 2023

Fleid commented Feb 15, 2023