Allow different jdbc metadata cache ttls for schema and tables lists #16090

atanasenko · 2023-02-13T12:47:25Z

Description

Missing table metadata can be configured to not get cached (cacheMissing), but that doesn't apply to schema and table name lists.

In certain cases it might be beneficial to shorten the list cache ttl for them to reflect the added schemas/tables faster, while specific table caches will automatically reload tables that were missing before.

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

hashhar · 2023-02-13T13:35:38Z

This is making configuration very complex over time. What is the actual problem we are trying to solve? Do we want to notice newly created objects faster? Or is the goal something else?

hashhar · 2023-02-13T13:37:02Z

Missing table metadata can be configured to not get cached (cacheMissing), but that doesn't apply to schema and table name lists.

To me it sounds like this is the solution we should implement - it would solve the problem in a simpler way and also make the config apply consistently.

skrzypo987 · 2023-02-13T14:40:39Z

To me it sounds like this is the solution we should implement

How should that work? The list of objects is not a 0/1 situation. With a table it either exists or it doesn't. A list is a list. It doesn't show up when the table is created, but it is changed.

skrzypo987

Still some tests failing.
Other than that looks good

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

Praveen2112 · 2023-02-17T07:17:35Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

Can we remove this constructor use the other one at all places ?

Those options are now non-optional and will have to be equal to metadataCachintTtl in such cases. I think it's easier to have an overloaded constructor to indicate that.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

Praveen2112 · 2023-02-17T07:26:25Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

Can we split this into two methods - one for table and other for schema ?

The test ensures that schema and table names cache ttls are different, it won't do that if they are separate.

The test method is quite big and it does assert multiple operations like with and without cache. Or we could have schema name cache, table name cache and a test method with combined configuration.

Those calls are inter-dependendent. First of all the test ensure that schema and table caches are different, an then it tests the evolution of the cache over time. I can split it into separate 'stages' which should make it more readable.

Split it into multiple smaller sections and extracted to a separate assertions class

Can we use a DataProvider in this case ?

Not sure if the two values for boolean warrant a DataProvider, and it's not really data, it's two modes of operations and they should be expressed explcitly.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

Praveen2112 · 2023-02-20T09:51:29Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

The test method is quite big and it does assert multiple operations like with and without cache. Or we could have schema name cache, table name cache and a test method with combined configuration.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

Praveen2112 · 2023-02-21T08:43:18Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

Can we use a DataProvider in this case ?

Praveen2112 · 2023-02-21T09:40:46Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

Since cacheMissing works only at a table level - do we need to mix up with the schema related tests. We could configure some default value of schema cache ttl and create a JDBC client. WDYT ?

I'm testing a usecase where a newly added table gets reflected in metadata lists faster than the general table cache ttl.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

Praveen2112 · 2023-02-27T08:07:59Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

If we are asserting only the schema and table names do we need this assertion related to tableHandleByName.

It's needed to make sure that metadata caches have separate ttl from individual table caches.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

findepi · 2023-02-27T10:57:25Z

@atanasenko please fork with @sopel39 to ensure this is consistent with recent Hive connector metadata caching changes design

atanasenko · 2023-02-27T13:07:48Z

Hive connector metadata caching changes design

@findepi those changes in #15811 seem to be related to extracting a separate ttl for stats, but here I'm separating some of the metadata, cache and individual table metadata are untouched.

Praveen2112

LGTM.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

* Extract assertTableHandleByNameCache and assertTableHandleByQueryCache * Align enum import with other methods

hashhar

skimmed, will do another pass

hashhar · 2023-03-02T14:23:06Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java

Does it make sense to cap this to be lower than metadata cache ttl? Listing operations will see a table but there would be no metadata cached for it and the table might've been dropped already.

Lists and specific metadata objects are loaded at different times too, sot this scenario can still happen.

@hashhar Theoretically, there could be some situations when users want to cache lists of schemas/tables for a longer amount of time (very big number of entries which do not change over time), but still let individual table data get reloaded.
This is a generic config after all.

That's a fair point.

ssheikin · 2023-03-02T14:51:56Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java

+        assertTableNamesCache(cachingJdbcClient)
+                .misses(1)
+                .loads(1)
+                .afterRunning(() -> {


I know it's preexisting, but
Should/can lambda have a parameter client which is received from assertTableNamesCache cachingJdbcClient?

Missing table metadata can be configured to not get cached (`cacheMissing`), but that doesn't apply to schema and table name lists. In certain cases it might be beneficial to shorten the list cache ttl for them to reflect the added schemas/tables faster, while specific table caches will automatically reload tables that were missing before.

hashhar

LGTM % whether we want to cap the list cache TTL to be lower than metadata cache ttl

hashhar

LGTM from me.

colebow · 2023-03-03T17:48:22Z

Does this need release notes? cc @kokosing

kokosing · 2023-03-03T20:25:21Z

Yes, thank you for asking.

JDBC connectors
*  Introduce `metadata.schemas.cache-ttl` and `metadata.tables.cache-ttl` configuration properties to control how long metadata information about tables and schemas should be kept in cache memory

CC: @atanasenko

cla-bot bot added the cla-signed label Feb 13, 2023

atanasenko requested review from findepi and skrzypo987 February 13, 2023 12:53

kokosing requested a review from hashhar February 13, 2023 13:19

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch 2 times, most recently from 66c39ac to 0b35c0e Compare February 15, 2023 13:51

skrzypo987 reviewed Feb 16, 2023

View reviewed changes

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch 2 times, most recently from 434709d to 677d14f Compare February 16, 2023 15:12

Praveen2112 reviewed Feb 17, 2023

View reviewed changes

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from 677d14f to a04744f Compare February 17, 2023 15:18

atanasenko requested review from Praveen2112 and skrzypo987 February 17, 2023 15:21

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from a04744f to 0f7254c Compare February 17, 2023 16:05

Praveen2112 reviewed Feb 20, 2023

View reviewed changes

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from 0f7254c to edcd11f Compare February 20, 2023 13:55

atanasenko requested a review from Praveen2112 February 20, 2023 14:00

skrzypo987 approved these changes Feb 21, 2023

View reviewed changes

Praveen2112 reviewed Feb 21, 2023

View reviewed changes

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch 3 times, most recently from f25eeb0 to 0648a59 Compare February 23, 2023 12:21

Praveen2112 reviewed Feb 27, 2023

View reviewed changes

findepi requested a review from sopel39 February 27, 2023 10:57

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from 0648a59 to d69f36b Compare February 27, 2023 11:11

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from d69f36b to 1b25265 Compare February 27, 2023 13:25

Praveen2112 approved these changes Feb 28, 2023

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java Outdated Show resolved Hide resolved

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java Outdated Show resolved Hide resolved

atanasenko requested review from martint, ssheikin and vlad-lyutenko March 2, 2023 10:27

atanasenko added 4 commits March 2, 2023 12:28

Expose schema name cache stats

55cd59b

Additional test coverage for tableNames cache

807255d

Minor cleanup in TestCachingJdbcClient

73015f2

* Extract assertTableHandleByNameCache and assertTableHandleByQueryCache * Align enum import with other methods

Use validation annotations in TestBaseJdbcConfig

46afd66

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from 1b25265 to bb45631 Compare March 2, 2023 10:32

hashhar reviewed Mar 2, 2023

View reviewed changes

ssheikin approved these changes Mar 2, 2023

View reviewed changes

atanasenko force-pushed the at/jdbc-metadata-cache-ttl branch from bb45631 to 215ed86 Compare March 2, 2023 16:05

hashhar approved these changes Mar 3, 2023

View reviewed changes

kokosing approved these changes Mar 3, 2023

View reviewed changes

kokosing merged commit 5d15ca4 into trinodb:master Mar 3, 2023

github-actions bot added this to the 409 milestone Mar 3, 2023

colebow mentioned this pull request Mar 3, 2023

Add Trino 409 release notes #16335

Merged

atanasenko deleted the at/jdbc-metadata-cache-ttl branch March 5, 2023 10:06

Allow different jdbc metadata cache ttls for schema and tables lists #16090

Allow different jdbc metadata cache ttls for schema and tables lists #16090

Uh oh!

Conversation

atanasenko commented Feb 13, 2023

Description

Additional context and related issues

Release notes

Uh oh!

hashhar commented Feb 13, 2023

Uh oh!

hashhar commented Feb 13, 2023

Uh oh!

skrzypo987 commented Feb 13, 2023

Uh oh!

skrzypo987 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi commented Feb 27, 2023

Uh oh!

atanasenko commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Praveen2112 left a comment

Choose a reason for hiding this comment

Uh oh!

atanasenko commented Feb 27, 2023 •

edited

Loading

kokosing commented Mar 3, 2023 •

edited

Loading