adding snowflake connector open connection retries #6
Conversation
There was a problem hiding this comment.
Thanks so much for the PR @mhmcdonald! A few questions from me. All in all, I'd be happy to make this change, at the (relatively small) cost of slightly longer delays to uncover obvious connection failures (e.g. due to user error).
|
|
||
| connection.handle = handle | ||
| connection.state = 'open' | ||
| except snowflake.connector.errors.Error as e: |
There was a problem hiding this comment.
I'm glad we're catching an exception as defined by snowflake.connector. It looks like they define some more specific error codes for retry. Should we try to be more precise here, and check whether the error code is retryable? Or better to always retry on all connections? I imagine the latter is better, just figured it's worth double-checking.
| connection.state = 'open' | ||
| except snowflake.connector.errors.Error as e: | ||
| error = None | ||
| for attempt in range(1, 4): # retry opening the connection twice after encountering an error |
There was a problem hiding this comment.
Should we make the count of retries configurable, with a reasonable default, rather than hard-coding 3? Can we imagine a reason why someone might want a different number (or to disable retries entirely)?
For context:
- We'll retry getting an auth token up to 20 times
- We make this configurable in dbt-spark via
retry_all+connect_retries
…ake into feature/issue-2482
There was a problem hiding this comment.
@mhmcdonald Thanks for the updates to this PR. This feels a bit like a sledgehammer-sized solution for a nail-sized problem, but since we can't readily identify the nature of the nail, it may just be what we need for the time being.
I just merged #43, which fixed a few integration tests that were affected by changes in dbt-core. If you can pull latest changes from main, you should be able to get tests passing.
Could you add an entry to CHANGELOG.md (dbt-snowflake 1.0.0 (Release TBD) > Features), and add yourself to the list of contributors?
Thanks for looking at this again @jtcohen6 - I agree that it's not the most elegant solution, but it's the best that I could come up with for now. I think it will relieve some headaches that the jwt token issue has caused for some of us Snowflake users. I went ahead and added the updates to the CHANGELOG.md. Because this PR includes new optional parameters to profile.yml file for Snowflake, is there documentation that I would need to update to get it reflected in dbt docs? |
There was a problem hiding this comment.
@mhmcdonald Thanks for the contribution! I'm going to get this in before we cut v1.0.0rc1 :)
Indeed, we'll want to update https://docs.getdbt.com/reference/warehouse-profiles/snowflake-profile. You can help by opening a PR here, against the next branch, since this will only be available in a prerelease version for now.
resolves #
This will help with the invalid JWT token issue described in this ticket: https://github.com/dbt-labs/dbt/issues/2482
We've run into this same issue at my company, and for non-dbt workflows, adding a simple retry to the connection has worked great for us. In our case, this is not because of queueing /overusing threads as described in 2482. There are a number of reasons why this issue can happen.
Description
This pull request adds a simple retry to the open method of SnowflakeConnectionManager.
I have run the unit tests and everything still passes.
At my company, we've seen this invalid JWT token issue pop up more recently in the last week. It recently caused our production flow run to fail. Also, anecdotally on dbt slack, others have been encountering it more recently, which makes me think the cause is on the Snowflake side. See the thread here.
Checklist
CHANGELOG.mdand added information about my change to the "dbt-snowflake next" section.