-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Bugfix/issue 3350 snowflake non json response #3365
Bugfix/issue 3350 snowflake non json response #3365
Conversation
yo @matt-winkler appreciate you opening the PR! One thing I wanted to follow up on - do either you or @barberscott understand the root cause of this issue? Scott indicated that this could happen for "a variety of reasons", which is fair and true, but do we feel confident that retrying is going to be a successful strategy? It's possible that this query will fail repeatedly and actually:
Dropping some code suggestions inline, but did want to better understand if we have a sense of the root cause at this point! |
Yep that's good feedback @drewbanin - am looking to get something moving here based on the sketch Jeremy provided in the issue. Agree with root causing it further before merging. One comment is that we'd have a 1 second delay rather than 20 with this (50 ms per loop); let me know if I'm mentally off somewhere in this. |
Thoughts:
|
@barberscott yeah! I'm with you. I think it would be ok if we omitted a config for this backoff/retry here - but I also think we should 100% log the status code of the response and the error message that comes back. @matt-winkler think I skimmed the logic wrong - you're all good :). I'm going to take a pass and drop some comments in the code |
|
||
result = requests.post(token_url, headers=headers, data=data) | ||
|
||
if result.headers.get('content-type') == 'application/json': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking the content-type is totally reasonable, but I think a more robust approach would try/catch the .json()
method call. That might look like (pseudo code):
result_json = None
for i in range(max_iter):
result = requests.post(token_url, headers=headers, data=data)
try:
result_json = result.json()
break
except ValueError as e:
message = response.text() # or something like that
logger.debug(f"Got a non-json response ({result.status}): {e}, message: {message}")
time.sleep(0.05)
if result_json is None:
# We'd want to raise an error here, otherwise `result_json['access_token']` below will fail
pass
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right on. The only reason I recommended checking the content-type instead of try/except in my issue comment was from having read about the potential slowness of the exception-catching approach. On further reading/thinking, that should only matter for very large request responses, which this isn't.
Thanks for the help all! This change looks solid to me. I'm glad we're going to debug-log the non-JSON response, so we can try to get to the bottom of this. @matt-winkler There are just some flake8 (code style) issues, and then I'd be happy to get this in before the next v0.20 prerelease:
|
Hey @jtcohen6 I updated the code after linting and did a test run using this large dbt project, during which I did not see connection errors. However, I'm not seeing a functional difference between running on the code in |
ok - if we're having a hard time reproducing this one locally, then i think we should just merge this one when we feel good about the change is. A retry & better logging never hurt anyone. Removing myself from review and will let y'all take it from here :) |
resolves #3350
Description
Runs a while loop for 1 second to validate whether
content-type
in the oauth response from Snowflake is JSON.Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.