From 8e60306ef8595dcfaa0b0ff051638f885706bf57 Mon Sep 17 00:00:00 2001 From: Colin Date: Tue, 3 Dec 2024 20:38:05 -0800 Subject: [PATCH 01/16] support retrying interface exceptions during query execution --- dbt/adapters/redshift/connections.py | 23 +++++++++++++++++------ dev-requirements.txt | 2 +- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index d93847634..fec1d317e 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -86,6 +86,10 @@ class RedshiftSSLMode(StrEnum): } +def _exponential_backoff(attempt: int): + return attempt * attempt + + @dataclass class RedshiftSSLConfig(dbtClassMixin, Replaceable): # type: ignore ssl: bool = True @@ -439,9 +443,6 @@ def open(cls, connection): credentials = connection.credentials - def exponential_backoff(attempt: int): - return attempt * attempt - retryable_exceptions = [ redshift_connector.OperationalError, redshift_connector.DatabaseError, @@ -454,7 +455,7 @@ def exponential_backoff(attempt: int): connect=get_connection_method(credentials), logger=logger, retry_limit=credentials.retries, - retry_timeout=exponential_backoff, + retry_timeout=_exponential_backoff, retryable_exceptions=retryable_exceptions, ) open_connection.backend_pid = cls._get_backend_pid(open_connection) # type: ignore @@ -485,6 +486,7 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False): self._initialize_sqlparse_lexer() queries = sqlparse.split(sql) + conn = self.get_thread_connection() for query in queries: # Strip off comments from the current query without_comments = re.sub( @@ -496,12 +498,21 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False): if without_comments == "": continue + retryable_exceptions = [ + redshift_connector.InterfaceError, + ] + connection, cursor = super().add_query( - query, auto_begin, bindings=bindings, abridge_sql_log=abridge_sql_log + query, + auto_begin, + bindings=bindings, + abridge_sql_log=abridge_sql_log, + retryable_exceptions=retryable_exceptions, + retry_limit=conn.credentials.retries, + retry_timeout=_exponential_backoff, ) if cursor is None: - conn = self.get_thread_connection() conn_name = conn.name if conn and conn.name else "" raise DbtRuntimeError(f"Tried to run invalid SQL: {sql} on {conn_name}") diff --git a/dev-requirements.txt b/dev-requirements.txt index 5a8b7f23f..38cba8253 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,6 +1,6 @@ # install latest changes in dbt-core + dbt-postgres git+https://github.com/dbt-labs/dbt-core.git#subdirectory=core -git+https://github.com/dbt-labs/dbt-adapters.git +git+https://github.com/dbt-labs/dbt-adapters.git@addQueryRetries git+https://github.com/dbt-labs/dbt-adapters.git#subdirectory=dbt-tests-adapter git+https://github.com/dbt-labs/dbt-common.git git+https://github.com/dbt-labs/dbt-postgres.git From 4760c8ea850bba3efece7511517a2c4e68dab2de Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Wed, 4 Dec 2024 18:51:53 -0800 Subject: [PATCH 02/16] Fix conn acquisition failure bug and make unit tests conform to new spec. --- dbt/adapters/redshift/connections.py | 4 ++-- tests/unit/test_query.py | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index fec1d317e..d740f752c 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -486,7 +486,6 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False): self._initialize_sqlparse_lexer() queries = sqlparse.split(sql) - conn = self.get_thread_connection() for query in queries: # Strip off comments from the current query without_comments = re.sub( @@ -508,11 +507,12 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False): bindings=bindings, abridge_sql_log=abridge_sql_log, retryable_exceptions=retryable_exceptions, - retry_limit=conn.credentials.retries, + retry_limit=self.profile.credentials.retries, retry_timeout=_exponential_backoff, ) if cursor is None: + conn = self.get_thread_connection() conn_name = conn.name if conn and conn.name else "" raise DbtRuntimeError(f"Tried to run invalid SQL: {sql} on {conn_name}") diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index ff8076215..9b50a179f 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -1,3 +1,5 @@ +import redshift_connector + from multiprocessing import get_context from unittest import TestCase, mock @@ -99,11 +101,20 @@ def test_execute_without_fetch(self): def test_add_query_success(self): cursor = mock.Mock() - with mock.patch.object(SQLConnectionManager, "add_query") as mock_add_query: + with ( + mock.patch.object(SQLConnectionManager, "add_query") as mock_add_query, + mock.patch("dbt.adapters.redshift.connections._exponential_backoff") as mock_func, + ): mock_add_query.return_value = None, cursor self.adapter.connections.add_query("select * from test3") mock_add_query.assert_called_once_with( - "select * from test3", True, bindings=None, abridge_sql_log=False + "select * from test3", + True, + bindings=None, + abridge_sql_log=False, + retryable_exceptions=[redshift_connector.InterfaceError], + retry_limit=1, + retry_timeout=mock_func, ) def test_add_query_with_no_cursor(self): From dd10db0e87df8199688f67ab046c028361c6b489 Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Wed, 4 Dec 2024 18:57:50 -0800 Subject: [PATCH 03/16] Add changelog. --- .changes/unreleased/Under the Hood-20241204-185729.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Under the Hood-20241204-185729.yaml diff --git a/.changes/unreleased/Under the Hood-20241204-185729.yaml b/.changes/unreleased/Under the Hood-20241204-185729.yaml new file mode 100644 index 000000000..2f3d76a88 --- /dev/null +++ b/.changes/unreleased/Under the Hood-20241204-185729.yaml @@ -0,0 +1,6 @@ +kind: Under the Hood +body: Add retry logic for retryable exceptions +time: 2024-12-04T18:57:29.925299-08:00 +custom: + Author: versusfacit + Issue: "960" From 3e34ae96dae934a416c17f4410ee240aed8a9b61 Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Mon, 16 Dec 2024 04:01:46 -0800 Subject: [PATCH 04/16] Add another exception to the list. Others are not called according to redshift's official docs See more here: https://github.com/aws/amazon-redshift-python-driver/blob/master/redshift_connector/error.py --- dbt/adapters/redshift/connections.py | 15 +++++---------- tests/unit/test_query.py | 11 +++++------ 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index d740f752c..a23563c72 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -86,10 +86,6 @@ class RedshiftSSLMode(StrEnum): } -def _exponential_backoff(attempt: int): - return attempt * attempt - - @dataclass class RedshiftSSLConfig(dbtClassMixin, Replaceable): # type: ignore ssl: bool = True @@ -443,19 +439,18 @@ def open(cls, connection): credentials = connection.credentials - retryable_exceptions = [ + retryable_exceptions = ( redshift_connector.OperationalError, redshift_connector.DatabaseError, redshift_connector.DataError, redshift_connector.InterfaceError, - ] + ) open_connection = cls.retry_connection( connection, connect=get_connection_method(credentials), logger=logger, retry_limit=credentials.retries, - retry_timeout=_exponential_backoff, retryable_exceptions=retryable_exceptions, ) open_connection.backend_pid = cls._get_backend_pid(open_connection) # type: ignore @@ -497,9 +492,10 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False): if without_comments == "": continue - retryable_exceptions = [ + retryable_exceptions = ( redshift_connector.InterfaceError, - ] + redshift_connector.InternalError, + ) connection, cursor = super().add_query( query, @@ -508,7 +504,6 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False): abridge_sql_log=abridge_sql_log, retryable_exceptions=retryable_exceptions, retry_limit=self.profile.credentials.retries, - retry_timeout=_exponential_backoff, ) if cursor is None: diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 9b50a179f..1de60c3b3 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -101,10 +101,7 @@ def test_execute_without_fetch(self): def test_add_query_success(self): cursor = mock.Mock() - with ( - mock.patch.object(SQLConnectionManager, "add_query") as mock_add_query, - mock.patch("dbt.adapters.redshift.connections._exponential_backoff") as mock_func, - ): + with (mock.patch.object(SQLConnectionManager, "add_query") as mock_add_query,): mock_add_query.return_value = None, cursor self.adapter.connections.add_query("select * from test3") mock_add_query.assert_called_once_with( @@ -112,9 +109,11 @@ def test_add_query_success(self): True, bindings=None, abridge_sql_log=False, - retryable_exceptions=[redshift_connector.InterfaceError], + retryable_exceptions=( + redshift_connector.InterfaceError, + redshift_connector.InternalError, + ), retry_limit=1, - retry_timeout=mock_func, ) def test_add_query_with_no_cursor(self): From cf9a64e8b99333294bae0c86f78c692619457637 Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Mon, 16 Dec 2024 04:14:09 -0800 Subject: [PATCH 05/16] Update branch reference. --- hatch.toml | 2 +- pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hatch.toml b/hatch.toml index 972c3609e..0e631b88f 100644 --- a/hatch.toml +++ b/hatch.toml @@ -9,7 +9,7 @@ packages = ["dbt"] [envs.default] dependencies = [ - "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git @ addQueryRetries", + "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git@addQueryRetries", "dbt-common @ git+https://github.com/dbt-labs/dbt-common.git", "dbt-tests-adapter @ git+https://github.com/dbt-labs/dbt-adapters.git#subdirectory=dbt-tests-adapter", "dbt-core @ git+https://github.com/dbt-labs/dbt-core.git#subdirectory=core", diff --git a/pyproject.toml b/pyproject.toml index 210dbe478..de5edab31 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ classifiers = [ ] dependencies = [ "dbt-common>=1.10,<2.0", - "dbt-adapters>=1.7,<2.0", + "dbt-adapters>=1.7,<2.0@addQueryRetries", "dbt-postgres>=1.8,<1.10", # dbt-redshift depends deeply on this package. it does not follow SemVer, therefore there have been breaking changes in previous patch releases # Pin to the patch or minor version, and bump in each new minor version of dbt-redshift. From cf25dc4e4595963e945950901348d08b05e8f23a Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Mon, 16 Dec 2024 04:17:41 -0800 Subject: [PATCH 06/16] Fix branch ref. --- pyproject.toml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index de5edab31..532e84c99 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ classifiers = [ ] dependencies = [ "dbt-common>=1.10,<2.0", - "dbt-adapters>=1.7,<2.0@addQueryRetries", + "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git@addQueryRetries", "dbt-postgres>=1.8,<1.10", # dbt-redshift depends deeply on this package. it does not follow SemVer, therefore there have been breaking changes in previous patch releases # Pin to the patch or minor version, and bump in each new minor version of dbt-redshift. @@ -55,3 +55,6 @@ filterwarnings = [ "ignore:.*'soft_unicode' has been renamed to 'soft_str'*:DeprecationWarning", "ignore:unclosed file .*:ResourceWarning", ] + +[tool.hatch.metadata] +allow-direct-references = true From f88cbcb259992f4c2d4a6807cdddd904e96c6265 Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Mon, 16 Dec 2024 14:00:52 -0800 Subject: [PATCH 07/16] Fix typo from prior solution. --- tests/unit/test_query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 1de60c3b3..c625e9a7f 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -101,7 +101,7 @@ def test_execute_without_fetch(self): def test_add_query_success(self): cursor = mock.Mock() - with (mock.patch.object(SQLConnectionManager, "add_query") as mock_add_query,): + with mock.patch.object(SQLConnectionManager, "add_query") as mock_add_query: mock_add_query.return_value = None, cursor self.adapter.connections.add_query("select * from test3") mock_add_query.assert_called_once_with( From b03b23d23c5af806eaea1259d95b6c7aa933ba1f Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Mon, 16 Dec 2024 14:15:05 -0800 Subject: [PATCH 08/16] Restore exponential backoff for retries which IS on main --- dbt/adapters/redshift/connections.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index a23563c72..98c5a1db2 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -439,6 +439,9 @@ def open(cls, connection): credentials = connection.credentials + def exponential_backoff(attempt: int): + return attempt * attempt + retryable_exceptions = ( redshift_connector.OperationalError, redshift_connector.DatabaseError, @@ -451,6 +454,7 @@ def open(cls, connection): connect=get_connection_method(credentials), logger=logger, retry_limit=credentials.retries, + retry_timeout=exponential_backoff, retryable_exceptions=retryable_exceptions, ) open_connection.backend_pid = cls._get_backend_pid(open_connection) # type: ignore From 63c28b7cf1a249132797fb173743745bc895fdf4 Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Mon, 16 Dec 2024 16:02:06 -0800 Subject: [PATCH 09/16] remove backoff and align with 1 second retry window described in docs. --- dbt/adapters/redshift/connections.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 98c5a1db2..a23563c72 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -439,9 +439,6 @@ def open(cls, connection): credentials = connection.credentials - def exponential_backoff(attempt: int): - return attempt * attempt - retryable_exceptions = ( redshift_connector.OperationalError, redshift_connector.DatabaseError, @@ -454,7 +451,6 @@ def exponential_backoff(attempt: int): connect=get_connection_method(credentials), logger=logger, retry_limit=credentials.retries, - retry_timeout=exponential_backoff, retryable_exceptions=retryable_exceptions, ) open_connection.backend_pid = cls._get_backend_pid(open_connection) # type: ignore From 3fce90a2ff4ce6e68d5f19e8f9b329800682f1e3 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-rogers-dbt@users.noreply.github.com> Date: Tue, 17 Dec 2024 14:10:42 -0800 Subject: [PATCH 10/16] Update hatch.toml --- hatch.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hatch.toml b/hatch.toml index 0e631b88f..2811756a4 100644 --- a/hatch.toml +++ b/hatch.toml @@ -9,7 +9,7 @@ packages = ["dbt"] [envs.default] dependencies = [ - "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git@addQueryRetries", + "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git", "dbt-common @ git+https://github.com/dbt-labs/dbt-common.git", "dbt-tests-adapter @ git+https://github.com/dbt-labs/dbt-adapters.git#subdirectory=dbt-tests-adapter", "dbt-core @ git+https://github.com/dbt-labs/dbt-core.git#subdirectory=core", From 56dba38d5e6151bf1eebfcc4895a52cb826408e1 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-rogers-dbt@users.noreply.github.com> Date: Tue, 17 Dec 2024 14:11:07 -0800 Subject: [PATCH 11/16] Update pyproject.toml --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 532e84c99..91802beb7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ classifiers = [ ] dependencies = [ "dbt-common>=1.10,<2.0", - "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git@addQueryRetries", + "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git", "dbt-postgres>=1.8,<1.10", # dbt-redshift depends deeply on this package. it does not follow SemVer, therefore there have been breaking changes in previous patch releases # Pin to the patch or minor version, and bump in each new minor version of dbt-redshift. From 81571383a9bad7d4f612e2b0947282e2abc9c8c8 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-rogers-dbt@users.noreply.github.com> Date: Tue, 17 Dec 2024 14:11:50 -0800 Subject: [PATCH 12/16] Update pyproject.toml --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 91802beb7..8aaf71f55 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ classifiers = [ ] dependencies = [ "dbt-common>=1.10,<2.0", - "dbt-adapters @ git+https://github.com/dbt-labs/dbt-adapters.git", + "dbt-adapters>=1.7,<2.0", "dbt-postgres>=1.8,<1.10", # dbt-redshift depends deeply on this package. it does not follow SemVer, therefore there have been breaking changes in previous patch releases # Pin to the patch or minor version, and bump in each new minor version of dbt-redshift. From 043343bafffa4d95f012b89c8a1a2626c7f41fa9 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-rogers-dbt@users.noreply.github.com> Date: Tue, 17 Dec 2024 14:28:01 -0800 Subject: [PATCH 13/16] Update Under the Hood-20241204-185729.yaml --- .changes/unreleased/Under the Hood-20241204-185729.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/unreleased/Under the Hood-20241204-185729.yaml b/.changes/unreleased/Under the Hood-20241204-185729.yaml index 2f3d76a88..276308126 100644 --- a/.changes/unreleased/Under the Hood-20241204-185729.yaml +++ b/.changes/unreleased/Under the Hood-20241204-185729.yaml @@ -2,5 +2,5 @@ kind: Under the Hood body: Add retry logic for retryable exceptions time: 2024-12-04T18:57:29.925299-08:00 custom: - Author: versusfacit + Author: versusfacit colin-rogers-dbt Issue: "960" From 8b6349923e5930c854af752c589303f203b20cd6 Mon Sep 17 00:00:00 2001 From: Colin Date: Tue, 17 Dec 2024 15:22:30 -0800 Subject: [PATCH 14/16] remove tools.hatch.metadata ref --- hatch.toml | 3 --- pyproject.toml | 3 --- 2 files changed, 6 deletions(-) diff --git a/hatch.toml b/hatch.toml index 2811756a4..3a3990a6c 100644 --- a/hatch.toml +++ b/hatch.toml @@ -59,6 +59,3 @@ check-sdist = [ "pip freeze | grep dbt-redshift", ] docker-prod = "docker build -f docker/Dockerfile -t dbt-redshift ." - -[tool.hatch.metadata] -allow-direct-references = true diff --git a/pyproject.toml b/pyproject.toml index 8aaf71f55..210dbe478 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,6 +55,3 @@ filterwarnings = [ "ignore:.*'soft_unicode' has been renamed to 'soft_str'*:DeprecationWarning", "ignore:unclosed file .*:ResourceWarning", ] - -[tool.hatch.metadata] -allow-direct-references = true From 03dd1f79bdf217e405f975018f878a8f8f1d7290 Mon Sep 17 00:00:00 2001 From: Colin Date: Tue, 17 Dec 2024 15:23:47 -0800 Subject: [PATCH 15/16] update adapters lower bound --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 210dbe478..e68aa2607 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,7 +24,7 @@ classifiers = [ ] dependencies = [ "dbt-common>=1.10,<2.0", - "dbt-adapters>=1.7,<2.0", + "dbt-adapters>=1.11,<2.0", "dbt-postgres>=1.8,<1.10", # dbt-redshift depends deeply on this package. it does not follow SemVer, therefore there have been breaking changes in previous patch releases # Pin to the patch or minor version, and bump in each new minor version of dbt-redshift. From 058ced02ecab068894f5837e178fa7158aea8755 Mon Sep 17 00:00:00 2001 From: VersusFacit <67295367+VersusFacit@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:15:07 -0800 Subject: [PATCH 16/16] jumpstart CI