Skip to content

Add cache for jdbc metadata calls#20359

Merged
NikhilCollooru merged 1 commit intoprestodb:masterfrom
NikhilCollooru:xdbCaching
Jul 21, 2023
Merged

Add cache for jdbc metadata calls#20359
NikhilCollooru merged 1 commit intoprestodb:masterfrom
NikhilCollooru:xdbCaching

Conversation

@NikhilCollooru
Copy link
Contributor

Created custom build with changes and tested by shadowing prod queries.

== RELEASE NOTES ==
Jdbc Changes
* Add cache support for jdbc metadata calls. This can be enabled by configuring parameter `metadata-cache-ttl`, `metadata-cache-refresh-interval` and `metadata-cache-size`.

@NikhilCollooru NikhilCollooru requested a review from a team as a code owner July 21, 2023 03:28
@jainxrohit
Copy link
Contributor

Can you share some results what improvements did you see?

@NikhilCollooru
Copy link
Contributor Author

Can you share some results what improvements did you see?

Screenshot 2023-07-20 at 8 40 32 PM

@NikhilCollooru
Copy link
Contributor Author

The cache metrics can be seen like this
Screenshot 2023-07-20 at 8 42 11 PM

@jainxrohit
Copy link
Contributor

The cache metrics can be seen like this Screenshot 2023-07-20 at 8 42 11 PM

We need to add all these new metrics to admin dashboards.

Copy link
Contributor

@jainxrohit jainxrohit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

I think it would make sense to switch the control flow, so have CachingJdbcMetadata have JdbcMetadata as a delegate and implement all the methods that JdbcMetadata does, and call to the delegate for everything (sometimes a simple call, sometimes as part of loading into the cache).

a) this is how i think all of our other CachingXXX classes work
b) it means all of the actual implementation for getting the metadata is in one place, and the only thing that needs to know about what the cache supports is the CachingJdbcMetadata. The JdbcMetadata doesn't need to know which things it can get from the caching metadata (which only implements some things from the interface, but the whole connectormetada interface is available and has default implementaiton) and which things it needs to get itself

@NikhilCollooru
Copy link
Contributor Author

I think it would make sense to switch the control flow, so have CachingJdbcMetadata have JdbcMetadata as a delegate and implement all the methods that JdbcMetadata does, and call to the delegate for everything (sometimes a simple call, sometimes as part of loading into the cache).

a) this is how i think all of our other CachingXXX classes work b) it means all of the actual implementation for getting the metadata is in one place, and the only thing that needs to know about what the cache supports is the CachingJdbcMetadata. The JdbcMetadata doesn't need to know which things it can get from the caching metadata (which only implements some things from the interface, but the whole connectormetada interface is available and has default implementaiton) and which things it needs to get itself

The issue is that we need CachingJdbcMetadata to be singleton so that the cache inside it will be shared across queries. But JdbcMetadata is not singleton and an instance is created for every trasaction/query. So switching the flow will not work.

After offline discussion, we decided we will keep the flow same, but will CachingJdbcMetadata(renamed to JdbcMetadataCache) will just be a cache and NOT implement ConnectorMetadata.

@NikhilCollooru NikhilCollooru merged commit 7cd1a41 into prestodb:master Jul 21, 2023
@NikhilCollooru NikhilCollooru deleted the xdbCaching branch July 21, 2023 20:05
@wanglinsong wanglinsong mentioned this pull request Jul 27, 2023
28 tasks
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.

3 participants