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

feat: improve key for connector cache #1172

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Conversation

jackwotherspoon
Copy link
Collaborator

@jackwotherspoon jackwotherspoon commented Oct 3, 2024

Improve dictionary key for connector._cache.

Previously, key for dictionary was the Cloud SQL instance's connection name
(instance_connection_string).

cache = connector._cache[instance_connection_string]

However, there is differing logic for connecting with enable_iam_auth=True
vs without. This resulted in having to throw an error and having a customer
create two separate connector objects if they want to connect to a single
instance with both auto IAM AuthN and regular user/password.

This PR makes the dictionary key a tuple consisting of (instance_connection_string, enable_iam_auth).
This allows a single connector to cache two separate entries for the same Cloud SQL instance,
one for enable_iam_auth=True and one for enable_iam_auth=False.

cache = connector._cache[(instance_connection_string, enable_iam_auth)]

Closes #1013

@jackwotherspoon jackwotherspoon self-assigned this Oct 3, 2024
@jackwotherspoon jackwotherspoon requested a review from a team as a code owner October 3, 2024 14:34
@jackwotherspoon jackwotherspoon changed the title feat: improve connector cache keys feat: improve cache key for connector cache Oct 3, 2024
@jackwotherspoon jackwotherspoon changed the title feat: improve cache key for connector cache feat: improve key for connector cache Oct 3, 2024
Copy link
Contributor

@hessjcg hessjcg left a comment

Choose a reason for hiding this comment

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

Nice work, this looks good.

A couple of ideas to for additional work:

  • We need do to this for the Go connector, maybe the Node connector too, I haven't checked yet.
  • Let's consider if we should add all relevant configuration values to the key similar to how the Java connector does it. See ConnectorConfig.java
    • target priniciple & delegates
    • admin url
    • quota project
    • universe domain
    • refresh strategy
    • google credentials

@jackwotherspoon
Copy link
Collaborator Author

A couple of ideas to for additional work:

  • We need do to this for the Go connector, maybe the Node connector too, I haven't checked yet.

  • Let's consider if we should add all relevant configuration values to the key similar to how the Java connector does it. See ConnectorConfig.java

    • target priniciple & delegates
    • admin url
    • quota project
    • universe domain
    • refresh strategy
    • google credentials

@hessjcg Most (if not all) of these do not make sense given how the Python Connector works. It comes down to Connector level config vs Instance level config. Since enable_iam_auth is an instance level config, it makes sense to cache different entries in the cache.

However, for Connector level config attributes (credentials, HTTP client, etc), it should in fact be recommended to use a separate connector object. The Connector() initializes a single async HTTP client that is shared across all connections to all instances. So for things like admin url, quota project, universe domain, etc. We can not be switching them from one instance to the other.

@jackwotherspoon jackwotherspoon merged commit 066c14e into main Oct 7, 2024
15 checks passed
@jackwotherspoon jackwotherspoon deleted the improved-cache-key branch October 7, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve caching of Instance() configs
3 participants