From d3c409c0925e18c9c33a73ab1dd7aba466044204 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 15 Mar 2022 10:49:23 -0500 Subject: [PATCH 1/2] catch all requests exceptions to retry --- core/dbt/events/types.py | 12 +++++++++- core/dbt/utils.py | 14 +++++------ test/unit/test_core_dbt_utils.py | 41 ++++++++++---------------------- test/unit/test_events.py | 1 + 4 files changed, 31 insertions(+), 37 deletions(-) diff --git a/core/dbt/events/types.py b/core/dbt/events/types.py index 7fda8031145..5190a58f210 100644 --- a/core/dbt/events/types.py +++ b/core/dbt/events/types.py @@ -2346,7 +2346,7 @@ def message(self) -> str: class RetryExternalCall(DebugLevel): attempt: int max: int - code: str = "Z045" + code: str = "M020" def message(self) -> str: return f"Retrying external call. Attempt: {self.attempt} Max attempts: {self.max}" @@ -2384,6 +2384,15 @@ def message(self) -> str: return "Internal event buffer full. Earliest events will be dropped (FIFO)." +@dataclass +class RecordRetryException(DebugLevel): + exc: Exception + code: str = "M021" + + def message(self) -> str: + return f"External call exception: {self.exc}" + + # since mypy doesn't run on every file we need to suggest to mypy that every # class gets instantiated. But we don't actually want to run this code. # making the conditional `if False` causes mypy to skip it as dead code so @@ -2737,3 +2746,4 @@ def message(self) -> str: GeneralWarningMsg(msg="", log_fmt="") GeneralWarningException(exc=Exception(""), log_fmt="") EventBufferFull() + RecordRetryException(exc=Exception("")) diff --git a/core/dbt/utils.py b/core/dbt/utils.py index 84dbb81dc7d..ebf5addd8da 100644 --- a/core/dbt/utils.py +++ b/core/dbt/utils.py @@ -17,7 +17,7 @@ from contextlib import contextmanager from dbt.exceptions import ConnectionException from dbt.events.functions import fire_event -from dbt.events.types import RetryExternalCall +from dbt.events.types import RetryExternalCall, RecordRetryException from dbt import flags from enum import Enum from typing_extensions import Protocol @@ -600,19 +600,19 @@ def __contains__(self, name) -> bool: def _connection_exception_retry(fn, max_attempts: int, attempt: int = 0): """Attempts to run a function that makes an external call, if the call fails - on a connection error, timeout or decompression issue, it will be tried up to 5 more times. - See https://github.com/dbt-labs/dbt-core/issues/4579 for context on this decompression issues - specifically. + on a Requests exception or decompression issue (ReadError), it will be tried + up to 5 more times. All exceptions that Requests explicitly raises inherit from + requests.exceptions.RequestException. See https://github.com/dbt-labs/dbt-core/issues/4579 + for context on this decompression issues specifically. """ try: return fn() except ( - requests.exceptions.ConnectionError, - requests.exceptions.Timeout, - requests.exceptions.ContentDecodingError, + requests.exceptions.RequestException, ReadError, ) as exc: if attempt <= max_attempts - 1: + fire_event(RecordRetryException(exc=exc)) fire_event(RetryExternalCall(attempt=attempt, max=max_attempts)) time.sleep(1) _connection_exception_retry(fn, max_attempts, attempt + 1) diff --git a/test/unit/test_core_dbt_utils.py b/test/unit/test_core_dbt_utils.py index 2c6ec182b94..1deb8a77552 100644 --- a/test/unit/test_core_dbt_utils.py +++ b/test/unit/test_core_dbt_utils.py @@ -12,27 +12,17 @@ def test_connection_exception_retry_none(self): connection_exception_retry(lambda: Counter._add(), 5) self.assertEqual(1, counter) + def test_connection_exception_retry_success_requests_exception(self): + Counter._reset() + connection_exception_retry(lambda: Counter._add_with_requests_exception(), 5) + self.assertEqual(2, counter) # 2 = original attempt returned None, plus 1 retry + def test_connection_exception_retry_max(self): Counter._reset() with self.assertRaises(ConnectionException): connection_exception_retry(lambda: Counter._add_with_exception(), 5) self.assertEqual(6, counter) # 6 = original attempt plus 5 retries - def test_connection_exception_retry_success(self): - Counter._reset() - connection_exception_retry(lambda: Counter._add_with_limited_exception(), 5) - self.assertEqual(2, counter) # 2 = original attempt plus 1 retry - - def test_connection_timeout(self): - Counter._reset() - connection_exception_retry(lambda: Counter._add_with_timeout(), 5) - self.assertEqual(2, counter) # 2 = original attempt plus 1 retry - - def test_connection_exception_retry_success_none_response(self): - Counter._reset() - connection_exception_retry(lambda: Counter._add_with_none_exception(), 5) - self.assertEqual(2, counter) # 2 = original attempt returned None, plus 1 retry - def test_connection_exception_retry_success_failed_untar(self): Counter._reset() connection_exception_retry(lambda: Counter._add_with_untar_exception(), 5) @@ -44,25 +34,18 @@ class Counter(): def _add(): global counter counter+=1 - def _add_with_exception(): - global counter - counter+=1 - raise requests.exceptions.ConnectionError - def _add_with_limited_exception(): - global counter - counter+=1 - if counter < 2: - raise requests.exceptions.ConnectionError - def _add_with_timeout(): + # All exceptions that Requests explicitly raises inherit from + # requests.exceptions.RequestException so we want to make sure that raises plus one exception + # that inherit from it for sanity + def _add_with_requests_exception(): global counter counter+=1 if counter < 2: - raise requests.exceptions.Timeout - def _add_with_none_exception(): + raise requests.exceptions.RequestException + def _add_with_exception(): global counter counter+=1 - if counter < 2: - raise requests.exceptions.ContentDecodingError + raise requests.exceptions.ConnectionError def _add_with_untar_exception(): global counter counter+=1 diff --git a/test/unit/test_events.py b/test/unit/test_events.py index dfbc780a7e9..0b5e86d0e49 100644 --- a/test/unit/test_events.py +++ b/test/unit/test_events.py @@ -414,6 +414,7 @@ def MockNode(): IntegrationTestError(msg=''), IntegrationTestException(msg=''), EventBufferFull(), + RecordRetryException(exc=Exception('')), UnitTestInfo(msg=''), ] From 5cad46cb15fde651bf06355a86b2609cfc1ad823 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 15 Mar 2022 10:53:48 -0500 Subject: [PATCH 2/2] add changelog --- .changes/unreleased/Fixes-20220315-105331.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changes/unreleased/Fixes-20220315-105331.yaml diff --git a/.changes/unreleased/Fixes-20220315-105331.yaml b/.changes/unreleased/Fixes-20220315-105331.yaml new file mode 100644 index 00000000000..9e21f48768d --- /dev/null +++ b/.changes/unreleased/Fixes-20220315-105331.yaml @@ -0,0 +1,8 @@ +kind: Fixes +body: Catch all Requests Exceptions on deps install to attempt retries. Also log + the exceptions hit. +time: 2022-03-15T10:53:31.637963-05:00 +custom: + Author: emmyoop + Issue: "4849" + PR: "4865"