diff --git a/.changes/unreleased/Fixes-20220422-131227.yaml b/.changes/unreleased/Fixes-20220422-131227.yaml new file mode 100644 index 00000000000..02a3973a5cc --- /dev/null +++ b/.changes/unreleased/Fixes-20220422-131227.yaml @@ -0,0 +1,7 @@ +kind: Fixes +body: Fix retry logic to return values after initial try +time: 2022-04-22T13:12:27.239055-05:00 +custom: + Author: emmyoop + Issue: "5023" + PR: "5137" diff --git a/.changes/unreleased/Under the Hood-20220414-132206.yaml b/.changes/unreleased/Under the Hood-20220414-132206.yaml new file mode 100644 index 00000000000..b2794d6ec00 --- /dev/null +++ b/.changes/unreleased/Under the Hood-20220414-132206.yaml @@ -0,0 +1,7 @@ +kind: Under the Hood +body: Move package deprecation check outside of package cache +time: 2022-04-14T13:22:06.157579-05:00 +custom: + Author: emmyoop + Issue: "5068" + PR: "5069" diff --git a/core/dbt/clients/registry.py b/core/dbt/clients/registry.py index 32159e4178a..a322999c2ca 100644 --- a/core/dbt/clients/registry.py +++ b/core/dbt/clients/registry.py @@ -89,6 +89,15 @@ def _get(package_name, registry_base_url=None): fire_event(RegistryResponseExtraNestedKeys(response=response)) raise requests.exceptions.ContentDecodingError(error_msg, response=resp) + return response + + +_get_cached = memoized(_get_with_retries) + + +def package(package_name, registry_base_url=None) -> Dict[str, Any]: + # returns a dictionary of metadata for all versions of a package + response = _get_cached(package_name, registry_base_url) # Either redirectnamespace or redirectname in the JSON response indicate a redirect # redirectnamespace redirects based on package ownership # redirectname redirects based on package name @@ -107,16 +116,6 @@ def _get(package_name, registry_base_url=None): new_nwo = use_namespace + "/" + use_name deprecations.warn("package-redirect", old_name=package_name, new_name=new_nwo) - - return response - - -_get_cached = memoized(_get_with_retries) - - -def package(package_name, registry_base_url=None) -> Dict[str, Any]: - # returns a dictionary of metadata for all versions of a package - response = _get_cached(package_name, registry_base_url) return response["versions"] diff --git a/core/dbt/utils.py b/core/dbt/utils.py index 9ea0d426007..7115bc75472 100644 --- a/core/dbt/utils.py +++ b/core/dbt/utils.py @@ -614,6 +614,6 @@ def _connection_exception_retry(fn, max_attempts: int, attempt: int = 0): fire_event(RecordRetryException(exc=exc)) fire_event(RetryExternalCall(attempt=attempt, max=max_attempts)) time.sleep(1) - _connection_exception_retry(fn, max_attempts, attempt + 1) + return _connection_exception_retry(fn, max_attempts, attempt + 1) else: raise ConnectionException('External connection exception occurred: ' + str(exc)) diff --git a/test/unit/test_connection_retries.py b/test/unit/test_connection_retries.py new file mode 100644 index 00000000000..8b031ce5ab4 --- /dev/null +++ b/test/unit/test_connection_retries.py @@ -0,0 +1,59 @@ +import functools +import pytest +from requests.exceptions import RequestException +from dbt.exceptions import ConnectionException +from dbt.utils import _connection_exception_retry + + +def no_retry_fn(): + return "success" + + +class TestNoRetries: + def test_no_retry(self): + fn_to_retry = functools.partial(no_retry_fn) + result = _connection_exception_retry(fn_to_retry, 3) + + expected = "success" + + assert result == expected + + +def no_success_fn(): + raise RequestException("You'll never pass") + return "failure" + + +class TestMaxRetries: + def test_no_retry(self): + fn_to_retry = functools.partial(no_success_fn) + + with pytest.raises(ConnectionException): + _connection_exception_retry(fn_to_retry, 3) + + +def single_retry_fn(): + global counter + if counter == 0: + counter += 1 + raise RequestException("You won't pass this one time") + elif counter == 1: + counter += 1 + return "success on 2" + + return "How did we get here?" + + +class TestSingleRetry: + def test_no_retry(self): + global counter + counter = 0 + + fn_to_retry = functools.partial(single_retry_fn) + result = _connection_exception_retry(fn_to_retry, 3) + expected = "success on 2" + + # We need to test the return value here, not just that it did not throw an error. + # If the value is not being passed it causes cryptic errors + assert result == expected + assert counter == 2