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

Improve run times when using key pair auth by caching the private key #1110

Merged
merged 10 commits into from
Jul 11, 2024

Conversation

mikealfare
Copy link
Contributor

resolves #1082

Problem

We evaluate and validate the private key on every connection when using key pair auth despite the fact that the key does not change during a dbt run.

Solution

  • create a function that uses caching outside of the Credentials class and pass the parameters to this function
  • use functools.lru_cache if we're using Python 3.8
  • use functools.cache if we're using Python 3.9
  • remove the check on Python version when we retire Python 3.8 support

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@mikealfare mikealfare self-assigned this Jul 10, 2024
@cla-bot cla-bot bot added the cla:yes label Jul 10, 2024
@mikealfare mikealfare marked this pull request as ready for review July 10, 2024 21:48
@mikealfare mikealfare requested a review from a team as a code owner July 10, 2024 21:48
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Caching LGTM, with a couple of nits.
I'll let someone from Adapters provide a final review.

dbt/adapters/snowflake/connections.py Outdated Show resolved Hide resolved
dbt/adapters/snowflake/connections.py Outdated Show resolved Hide resolved
@@ -63,6 +72,58 @@
}


@cache
Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt Jul 11, 2024

Choose a reason for hiding this comment

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

nit: can we move these to their own module? Just want to keep connections.py from getting bigger if we don't need to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually want to move them into dbt-adapters long term. I thought about creating a separate module, but thought it would be unnecessary if we were going to move it later. I can add that here though if you think it's worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth it as it should make it cleaner to move later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved over only the two methods that are generic. I left the one that's Snowflake-specific (converting the key to DER format).

Along the same lines of making connections.py smaller, I actually think the Credentials class should be in its own module (probably named auth, even though that's what I moved these methods to). That's a separate PR, but it shouldn't take long to do if we want to do that.

@mikealfare mikealfare merged commit 1193a79 into main Jul 11, 2024
18 checks passed
@mikealfare mikealfare deleted the cache-key-pair-auth branch July 11, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Improve key-pair auth performance
3 participants