Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/dbt deps retry none answer #4225

Merged
merged 5 commits into from
Nov 8, 2021

Conversation

b-per
Copy link
Contributor

@b-per b-per commented Nov 8, 2021

Allow retries when the answer received after a dbt deps is None

resolves #4178

Description

Add the logic to handle the case when the answer received is None.
I picked the requests exception InvalidJSONError to handle the situation but I am not sure if another one would be more suitable.

I didn't add tests and wouldn't be too sure where to start if a test would need to be created. Happy to add one if it is required.

Checklist

  • 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 updated the CHANGELOG.md and added information about my change

Allow retries when the answer is None
Allow retries when the answer from dbt deps is None
@cla-bot cla-bot bot added the cla:yes label Nov 8, 2021
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @b-per! This looks great to me. I'll look to see if there's an easy place to add a test, but by itself the code is probably good to go.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Nov 8, 2021

Just a few small things from flake8 + mypy:

core/dbt/utils.py:618:100: E501 line too long (123 > 99 characters)
core/dbt/clients/registry.py:34:100: E501 line too long (104 > 99 characters)
core/dbt/utils.py:618: error: Module has no attribute "InvalidJSONError"

I didn't add tests and wouldn't be too sure where to start if a test would need to be created. Happy to add one if it is required.

I think you could add a test to test/unit/test_core_dbt_utils.py, which was the test added in #3729. Something like:

    def test_connection_exception_retry_success(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

class Counter():
    ...
    def _add_with_none_exception():
        global counter
        counter+=1
        if counter < 2:
            raise requests.exceptions.InvalidJSONError

If you have a chance to do that today, great! If not, I'll try to get this merged ahead of v1.0.0-rc1, which we're planning to cut very soon.

@b-per
Copy link
Contributor Author

b-per commented Nov 8, 2021

Thanks for the review @jtcohen6

I am not too sure why InvalidJSONError raises an error with mypy, the class exists in exceptions.py.

Looking at the other exceptions available I actually changed the one raised to ContentDecodingError which doesn't raise a mypy error.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Nov 8, 2021

@b-per I'm not sure why mypy raised that error either. It looks like InvalidJSONError was just added in v2.6.0, but that is indeed the version of requests installed in CI.

In any case, thanks for the quick follow-up! This is good to go

@jtcohen6 jtcohen6 merged commit f20e83a into dbt-labs:main Nov 8, 2021
@b-per b-per deleted the fix/dbt-deps-retry-none-answer branch November 8, 2021 11:40
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
* Fix issue #4178
Allow retries when the answer is None

* Include fix for #4178
Allow retries when the answer from dbt deps is None

* Add link to the PR

* Update exception and shorten line size

* Add test when dbt deps returns None

automatic commit by git-black, original commits:
  f20e83a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] dbt deps should keep retrying if registry response is None
2 participants