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

Ct 854/change overzealous connection closing #428

Merged
merged 18 commits into from
Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3f824d0
Add release_connection setting to not release the connection
joshuataylor Jul 20, 2022
e33c0dd
Merge branch 'main' into feature/connection-release
joshuataylor Jul 31, 2022
020e31b
Merge branch 'main' into feature/connection-release
McKnight-42 Aug 17, 2022
b2b7ba5
Merge branch 'main' into feature/connection-release
joshuataylor Aug 23, 2022
09e57a3
Merge branch 'main' into feature/connection-release
McKnight-42 Aug 25, 2022
1c23d7b
Merge branch 'main' into feature/connection-release
joshuataylor Oct 23, 2022
4327237
Merge branch 'main' into feature/connection-release
joshuataylor Oct 23, 2022
cccffea
remove space
joshuataylor Oct 23, 2022
f5863ed
Merge branch 'main' into feature/connection-release
McKnight-42 Nov 10, 2022
ca468fa
Merge branch 'feature/connection-release' into CT-854/change_overzeal…
VersusFacit Jan 31, 2023
8ec71a5
Simplify code and add profiles.yml integration.
VersusFacit Jan 31, 2023
7edf782
Add release_connection to unit tests
VersusFacit Jan 31, 2023
ff047a0
Add changelog
VersusFacit Jan 31, 2023
3614d92
Add warning for combination that can result in a bug.
VersusFacit Jan 31, 2023
3eb1073
Merge branch 'main' into CT-854/change_overzealous_connection_closing
VersusFacit Jan 31, 2023
70fcea8
Merge branch 'main' into CT-854/change_overzealous_connection_closing
VersusFacit Feb 7, 2023
a2b6b70
Rename config var with None default, change up tests. Remove warnings…
VersusFacit Feb 7, 2023
8b246ce
Merge branch 'main' into CT-854/change_overzealous_connection_closing
VersusFacit Feb 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20230131-022659.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Add configurable setting for keeping connections open on Snowflake
time: 2023-01-31T02:26:59.701589-08:00
custom:
Author: versusfacit joshuataylor
Issue: "854"
11 changes: 11 additions & 0 deletions dbt/adapters/snowflake/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class SnowflakeCredentials(Credentials):
retry_on_database_errors: bool = False
retry_all: bool = False
insecure_mode: Optional[bool] = False
reuse_connections: Optional[bool] = None

def __post_init__(self):
if self.authenticator != "oauth" and (
Expand Down Expand Up @@ -151,6 +152,7 @@ def auth_args(self):
result["client_store_temporary_credential"] = True
# enable mfa token cache for linux
result["client_request_mfa_token"] = True
result["reuse_connections"] = self.reuse_connections
result["private_key"] = self._get_private_key()
return result

Expand Down Expand Up @@ -486,3 +488,12 @@ def add_query(self, sql, auto_begin=True, bindings=None, abridge_sql_log=False):
)

return connection, cursor

def release(self) -> None:
"""Reuse connections by deferring release until adapter context manager in core
resets adapters. This cleanup_all happens before Python teardown. Idle connections
incur no costs while waiting in the connection pool."""
if self.profile.credentials.reuse_connections: # type: ignore
return
else:
super().release()
43 changes: 31 additions & 12 deletions tests/unit/test_snowflake_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ def test_client_session_keep_alive_false_by_default(self):
client_session_keep_alive=False, database='test_database',
role=None, schema='public', user='test_user',
warehouse='test_warehouse', private_key=None, application='dbt', insecure_mode=False,
session_parameters={}),
session_parameters={}, reuse_connections=None),
])

def test_client_session_keep_alive_true(self):
Expand All @@ -279,7 +279,7 @@ def test_client_session_keep_alive_true(self):
client_session_keep_alive=True, database='test_database',
role=None, schema='public', user='test_user',
warehouse='test_warehouse', private_key=None, application='dbt', insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

def test_user_pass_authentication(self):
Expand All @@ -298,7 +298,7 @@ def test_user_pass_authentication(self):
password='test_password', role=None, schema='public',
user='test_user', warehouse='test_warehouse', private_key=None,
application='dbt', insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

def test_authenticator_user_pass_authentication(self):
Expand All @@ -318,9 +318,9 @@ def test_authenticator_user_pass_authentication(self):
password='test_password', role=None, schema='public',
user='test_user', warehouse='test_warehouse',
authenticator='test_sso_url', private_key=None,
application='dbt', client_request_mfa_token=True,
application='dbt', client_request_mfa_token=True,
client_store_temporary_credential=True, insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

def test_authenticator_externalbrowser_authentication(self):
Expand All @@ -338,9 +338,9 @@ def test_authenticator_externalbrowser_authentication(self):
client_session_keep_alive=False, database='test_database',
role=None, schema='public', user='test_user',
warehouse='test_warehouse', authenticator='externalbrowser',
private_key=None, application='dbt', client_request_mfa_token=True,
private_key=None, application='dbt', client_request_mfa_token=True,
client_store_temporary_credential=True, insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

def test_authenticator_oauth_authentication(self):
Expand All @@ -359,9 +359,9 @@ def test_authenticator_oauth_authentication(self):
client_session_keep_alive=False, database='test_database',
role=None, schema='public', user='test_user',
warehouse='test_warehouse', authenticator='oauth', token='my-oauth-token',
private_key=None, application='dbt', client_request_mfa_token=True,
private_key=None, application='dbt', client_request_mfa_token=True,
client_store_temporary_credential=True, insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

@mock.patch('dbt.adapters.snowflake.SnowflakeCredentials._get_private_key', return_value='test_key')
Expand All @@ -383,7 +383,7 @@ def test_authenticator_private_key_authentication(self, mock_get_private_key):
role=None, schema='public', user='test_user',
warehouse='test_warehouse', private_key='test_key',
application='dbt', insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

@mock.patch('dbt.adapters.snowflake.SnowflakeCredentials._get_private_key', return_value='test_key')
Expand All @@ -405,7 +405,7 @@ def test_authenticator_private_key_authentication_no_passphrase(self, mock_get_p
role=None, schema='public', user='test_user',
warehouse='test_warehouse', private_key='test_key',
application='dbt', insecure_mode=False,
session_parameters={})
session_parameters={}, reuse_connections=None)
])

def test_query_tag(self):
Expand All @@ -422,7 +422,26 @@ def test_query_tag(self):
password='test_password', role=None, schema='public',
user='test_user', warehouse='test_warehouse', private_key=None,
application='dbt', insecure_mode=False,
session_parameters={"QUERY_TAG": "test_query_tag"})
session_parameters={"QUERY_TAG": "test_query_tag"}, reuse_connections=None)
])

def test_reuse_connections_with_keep_alive(self):
self.config.credentials = self.config.credentials.replace(
reuse_connections=True,
client_session_keep_alive=True
)
self.adapter = SnowflakeAdapter(self.config)
conn = self.adapter.connections.set_connection_name(name='new_connection_with_new_config')

self.snowflake.assert_not_called()
conn.handle
self.snowflake.assert_has_calls([
mock.call(
account='test_account', autocommit=True,
client_session_keep_alive=True, database='test_database',
role=None, schema='public', user='test_user', warehouse='test_warehouse',
private_key=None, application='dbt', insecure_mode=False,
session_parameters={}, reuse_connections=True)
])


Expand Down