connection retry refactor, add defaults with exponential backoff

[nathaniel-may]

resolves #205

• Refactors retry logic to use retry_connection from core.
• Preserves existing behavior,
• Adds consistent defaults with other adpaters:
  • if connect_retries and connect_timeout are not specified: 1 retry after 1s
  • if connect_retries is specified but connect_timeout is not, it will use exponential backoff

This is a non-trivial change so I'll wait for sign off from @jtcohen6 before bothering to add the tests.

TODO

• add similar tests from postgres PR <- those are testing the library function in core so we're covered here too.

[jtcohen6]

👍

Also discussed: using connect_retries as the standard name everywhere (including Postgres/Redshift)

[jtcohen6]

IMO let's use retry_on_database_errors: True as our default. Users can optionally turn if off, or they can opt into all errors via retry_all.

(@nathaniel-may may also be inspired to dig further into Snowflake docs)

[nathaniel-may]

I found more sensible defaults by looking at the connector source.

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the dbt-core contributing guide, and the dbt-snowflake contriubuting guide.

[jeremyyeo]

Hey @nathaniel-may whilst you folks are working on this one... does this sync up the connect_timeout config of dbt-snowflake with all the likes of redshift/postgress adapter connect_timeout?

We recently put in a default of connect_timeout: 30 in dbt Cloud's snowflake profile (https://dbtlabs.atlassian.net/browse/SHP-588) since this config is unavailable to end users. When looking at some of the logs once this was deployed, we realized that this doesn't behave the same way as the other adapter's config (and then I double checked the docs¹ which does clearly state the behaviour here). As someone familiar with other adapters connect_timeout config, we automatically thought this config would behave the exact samesies.

This config for Snowflake's behaviour is:

If a connection terminates, wait connect_timeout seconds before retrying the connection.

As opposed to Redshift/PG's:

While connecting, wait connect_timeout seconds before dropping the connection.

Footnotes

1. https://docs.getdbt.com/reference/warehouse-profiles/snowflake-profile#retry_on_database_errors ↩

[nathaniel-may]

Hey @jeremyyeo! This is part of a larger effort to keep timeout config behavior to roughly the same standard:

The goal:

• default to one retry after 1s for retryable errors
• if the user configures a number of retries, use exponential backoff for retryable errors

Exceptions:

• Snowflake already had additional retry logic so this implementation is backwards compatible for users who have that already configured. To be specific- if the user has connect_timeout configured, retries will use that instead of exponential backoff.
• Postgres and Redshift will attempt to retry some non-retryable errors as well due to a psycopg2 limitation.
• Spark retries will remain unchanged and continue to attempt to retry ALL errors without exponential backoff. This is due to a spark driver limitation that doesn't allow us to easily reuse the library function for retries in core.
• BQ will remain unconfigurable. It will use its own exponential backoff strategy for only retryable errors as implemented in the gcp @Retry decorator. The behavior is roughly equivalent to configuring connect_retires = 6 on our other adapters because the default for bq is to stop retrying after a total of 120s but it will actually retry 10 times.

[jtcohen6]

LGTM! Excited to get this merged. Sorry for the delay with final review

[jtcohen6]

👍

[leahwicz]

@nathaniel-may can we merge this in for the end of the sprint and call it done?