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

Remove SQL db cache #41

Merged
merged 1 commit into from
Jan 12, 2022
Merged

Remove SQL db cache #41

merged 1 commit into from
Jan 12, 2022

Conversation

andresmgot
Copy link
Contributor

The interface method GetDB was returning a cached version of the db connection if the same datasource ID and connections args were passed. This is wrong because the db connection also depend on the datasource settings. This means that even after modifying the datasource settings, calling GetDB would return a db connection with wrong parameters.

To fix that, we would need to also take into consideration the datasource settings to store the db connection (or expose a different method that always creates a db connection) but truth is that we don't need to cache this connection at all. The GetDB method is only called in the Connect method of the datasource and this Connect method is only called when there is an actual need of a new connection:

  • When saving a datasource.
  • When running a query against a datasource for the first time.
  • When running a query with different connection arguments for the first time.

Because of that, I am removing that connection from the cache. Also note that the underlying AWS session (which handle the underlying connections) is already caching sessions so even if we call GetDB several times with the same arguments (e.g. clicking "Save datasource" several times), only one connection will be set up.

Fixes grafana/athena-datasource#105 (once released)

Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

makes sense to me!

@andresmgot andresmgot merged commit a098f70 into main Jan 12, 2022
@andresmgot andresmgot deleted the cacheDB branch January 12, 2022 10:38
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.

Changing the database in the datasource configuration takes no effect in the queries
2 participants