Skip to content

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

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.

Sign up for GitHub

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

Merged
kokosing merged 5 commits into from
Mar 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55cd59b
Expose schema name cache stats
atanasenko Feb 21, 2023
807255d
Additional test coverage for tableNames cache
atanasenko Feb 21, 2023
73015f2
Minor cleanup in TestCachingJdbcClient
atanasenko Feb 20, 2023
46afd66
Use validation annotations in TestBaseJdbcConfig
atanasenko Feb 20, 2023
215ed86
Allow different jdbc metadata cache ttls for schema and tables lists
atanasenko Feb 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 49 additions & 8 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,31 @@
import io.airlift.units.Duration;
import io.airlift.units.MinDuration;

import javax.annotation.PostConstruct;
import javax.validation.constraints.AssertTrue;
import javax.validation.constraints.Min;
import javax.validation.constraints.NotNull;
import javax.validation.constraints.Pattern;

import java.util.Optional;
import java.util.Set;

import static com.google.common.base.Strings.nullToEmpty;
import static java.lang.String.format;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static javax.validation.constraints.Pattern.Flag.CASE_INSENSITIVE;

public class BaseJdbcConfig
{
public static final String METADATA_CACHE_TTL = "metadata.cache-ttl";
public static final String METADATA_SCHEMAS_CACHE_TTL = "metadata.schemas.cache-ttl";
public static final String METADATA_TABLES_CACHE_TTL = "metadata.tables.cache-ttl";
public static final String METADATA_CACHE_MAXIMUM_SIZE = "metadata.cache-maximum-size";

private String connectionUrl;
private Set<String> jdbcTypesMappedToVarchar = ImmutableSet.of();
public static final Duration CACHING_DISABLED = new Duration(0, MILLISECONDS);
private Duration metadataCacheTtl = CACHING_DISABLED;
private Optional<Duration> schemaNamesCacheTtl = Optional.empty();
private Optional<Duration> tableNamesCacheTtl = Optional.empty();
private boolean cacheMissing;
public static final long DEFAULT_METADATA_CACHE_SIZE = 10000;
private long cacheMaximumSize = DEFAULT_METADATA_CACHE_SIZE;
Expand Down Expand Up @@ -87,6 +91,34 @@ public BaseJdbcConfig setMetadataCacheTtl(Duration metadataCacheTtl)
return this;
}

@NotNull
public Duration getSchemaNamesCacheTtl()
Copy link
Member

@hashhar hashhar Mar 2, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@atanasenko atanasenko Mar 2, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@atanasenko atanasenko Mar 3, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@hashhar hashhar Mar 3, 2023

Choose a reason for hiding this comment

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

That's a fair point.

{
return schemaNamesCacheTtl.orElse(metadataCacheTtl);
}

@Config(METADATA_SCHEMAS_CACHE_TTL)
@ConfigDescription("Determines how long schema names list information will be cached")
public BaseJdbcConfig setSchemaNamesCacheTtl(Duration schemaNamesCacheTtl)
{
this.schemaNamesCacheTtl = Optional.ofNullable(schemaNamesCacheTtl);
return this;
}

@NotNull
public Duration getTableNamesCacheTtl()
{
return tableNamesCacheTtl.orElse(metadataCacheTtl);
}

@Config(METADATA_TABLES_CACHE_TTL)
@ConfigDescription("Determines how long table names list information will be cached")
public BaseJdbcConfig setTableNamesCacheTtl(Duration tableNamesCacheTtl)
{
this.tableNamesCacheTtl = Optional.ofNullable(tableNamesCacheTtl);
return this;
}

public boolean isCacheMissing()
{
return cacheMissing;
Expand Down Expand Up @@ -114,12 +146,21 @@ public BaseJdbcConfig setCacheMaximumSize(long cacheMaximumSize)
return this;
}

@PostConstruct
public void validate()
@AssertTrue(message = METADATA_CACHE_TTL + " must be set to a non-zero value when " + METADATA_CACHE_MAXIMUM_SIZE + " is set")
public boolean isCacheMaximumSizeConsistent()
{
return !metadataCacheTtl.equals(CACHING_DISABLED) || cacheMaximumSize == BaseJdbcConfig.DEFAULT_METADATA_CACHE_SIZE;
}

@AssertTrue(message = METADATA_SCHEMAS_CACHE_TTL + " must not be set when " + METADATA_CACHE_TTL + " is not set")
public boolean isSchemaNamesCacheTtlConsistent()
{
return !metadataCacheTtl.equals(CACHING_DISABLED) || schemaNamesCacheTtl.isEmpty();
}

@AssertTrue(message = METADATA_TABLES_CACHE_TTL + " must not be set when " + METADATA_CACHE_TTL + " is not set")
public boolean isTableNamesCacheTtlConsistent()
{
if (metadataCacheTtl.equals(CACHING_DISABLED) && cacheMaximumSize != BaseJdbcConfig.DEFAULT_METADATA_CACHE_SIZE) {
throw new IllegalArgumentException(
format("%s must be set to a non-zero value when %s is set", METADATA_CACHE_TTL, METADATA_CACHE_MAXIMUM_SIZE));
}
return !metadataCacheTtl.equals(CACHING_DISABLED) || tableNamesCacheTtl.isEmpty();
}
}
72 changes: 54 additions & 18 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,15 @@ public CachingJdbcClient(
IdentityCacheMapping identityMapping,
BaseJdbcConfig config)
{
this(delegate, sessionPropertiesProviders, identityMapping, config.getMetadataCacheTtl(), config.isCacheMissing(), config.getCacheMaximumSize());
this(
delegate,
sessionPropertiesProviders,
identityMapping,
config.getMetadataCacheTtl(),
config.getSchemaNamesCacheTtl(),
config.getTableNamesCacheTtl(),
config.isCacheMissing(),
config.getCacheMaximumSize());
}

public CachingJdbcClient(
Expand All @@ -103,6 +111,26 @@ public CachingJdbcClient(
Duration metadataCachingTtl,
boolean cacheMissing,
long cacheMaximumSize)
{
this(delegate,
sessionPropertiesProviders,
identityMapping,
metadataCachingTtl,
metadataCachingTtl,
metadataCachingTtl,
cacheMissing,
cacheMaximumSize);
}

public CachingJdbcClient(
JdbcClient delegate,
Set<SessionPropertiesProvider> sessionPropertiesProviders,
IdentityCacheMapping identityMapping,
Duration metadataCachingTtl,
Duration schemaNamesCachingTtl,
Duration tableNamesCachingTtl,
boolean cacheMissing,
long cacheMaximumSize)
{
this.delegate = requireNonNull(delegate, "delegate is null");
this.sessionProperties = sessionPropertiesProviders.stream()
Expand All @@ -111,25 +139,27 @@ public CachingJdbcClient(
this.cacheMissing = cacheMissing;
this.identityMapping = requireNonNull(identityMapping, "identityMapping is null");

EvictableCacheBuilder<Object, Object> cacheBuilder = EvictableCacheBuilder.newBuilder()
.expireAfterWrite(metadataCachingTtl.toMillis(), MILLISECONDS)
.shareNothingWhenDisabled()
.recordStats();
long cacheSize = metadataCachingTtl.equals(CACHING_DISABLED)
// Disables the cache entirely
? 0
: cacheMaximumSize;

if (metadataCachingTtl.equals(CACHING_DISABLED)) {
// Disables the cache entirely
cacheBuilder.maximumSize(0);
}
else {
cacheBuilder.maximumSize(cacheMaximumSize);
}
schemaNamesCache = buildCache(cacheSize, schemaNamesCachingTtl);
tableNamesCache = buildCache(cacheSize, tableNamesCachingTtl);
tableHandlesByNameCache = buildCache(cacheSize, metadataCachingTtl);
tableHandlesByQueryCache = buildCache(cacheSize, metadataCachingTtl);
columnsCache = buildCache(cacheSize, metadataCachingTtl);
statisticsCache = buildCache(cacheSize, metadataCachingTtl);
}

schemaNamesCache = cacheBuilder.build();
tableNamesCache = cacheBuilder.build();
tableHandlesByNameCache = cacheBuilder.build();
tableHandlesByQueryCache = cacheBuilder.build();
columnsCache = cacheBuilder.build();
statisticsCache = cacheBuilder.build();
private static <K, V> Cache<K, V> buildCache(long cacheSize, Duration cachingTtl)
{
return EvictableCacheBuilder.newBuilder()
.maximumSize(cacheSize)
.expireAfterWrite(cachingTtl.toMillis(), MILLISECONDS)
.shareNothingWhenDisabled()
.recordStats()
.build();
}

@Override
Expand Down Expand Up @@ -583,6 +613,12 @@ private void invalidateColumnsCache(SchemaTableName table)
invalidateCache(columnsCache, key -> key.table.equals(table));
}

@VisibleForTesting
CacheStats getSchemaNamesCacheStats()
{
return schemaNamesCache.stats();
}

@VisibleForTesting
CacheStats getTableNamesCacheStats()
{
Expand Down
49 changes: 37 additions & 12 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestBaseJdbcConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@
import io.airlift.units.Duration;
import org.testng.annotations.Test;

import javax.validation.constraints.AssertTrue;

import java.util.Map;

import static io.airlift.configuration.testing.ConfigAssertions.assertFullMapping;
import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults;
import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults;
import static io.airlift.testing.ValidationAssertions.assertFailsValidation;
import static io.airlift.testing.ValidationAssertions.assertValidates;
import static java.util.concurrent.TimeUnit.MINUTES;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -40,6 +44,8 @@ public void testDefaults()
.setConnectionUrl(null)
.setJdbcTypesMappedToVarchar("")
.setMetadataCacheTtl(ZERO)
.setSchemaNamesCacheTtl(null)
.setTableNamesCacheTtl(null)
.setCacheMissing(false)
.setCacheMaximumSize(10000));
}
Expand All @@ -51,6 +57,8 @@ public void testExplicitPropertyMappings()
.put("connection-url", "jdbc:h2:mem:config")
.put("jdbc-types-mapped-to-varchar", "mytype,struct_type1")
.put("metadata.cache-ttl", "1s")
.put("metadata.schemas.cache-ttl", "2s")
.put("metadata.tables.cache-ttl", "3s")
.put("metadata.cache-missing", "true")
.put("metadata.cache-maximum-size", "5000")
.buildOrThrow();
Expand All @@ -59,6 +67,8 @@ public void testExplicitPropertyMappings()
.setConnectionUrl("jdbc:h2:mem:config")
.setJdbcTypesMappedToVarchar("mytype, struct_type1")
.setMetadataCacheTtl(new Duration(1, SECONDS))
.setSchemaNamesCacheTtl(new Duration(2, SECONDS))
.setTableNamesCacheTtl(new Duration(3, SECONDS))
.setCacheMissing(true)
.setCacheMaximumSize(5000);

Expand All @@ -81,19 +91,34 @@ public void testConnectionUrlIsValid()
@Test
public void testCacheConfigValidation()
{
BaseJdbcConfig explicitCacheSize = new BaseJdbcConfig()
assertValidates(new BaseJdbcConfig()
.setConnectionUrl("jdbc:h2:mem:config")
.setMetadataCacheTtl(new Duration(1, SECONDS))
.setCacheMaximumSize(5000);
explicitCacheSize.validate();
BaseJdbcConfig defaultCacheSize = new BaseJdbcConfig()
.setMetadataCacheTtl(new Duration(1, SECONDS));
defaultCacheSize.validate();
assertThatThrownBy(() -> new BaseJdbcConfig()
.setMetadataCacheTtl(ZERO)
.setCacheMaximumSize(100000)
.validate())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageMatching("metadata.cache-ttl must be set to a non-zero value when metadata.cache-maximum-size is set");
.setSchemaNamesCacheTtl(new Duration(2, SECONDS))
.setTableNamesCacheTtl(new Duration(3, SECONDS))
.setCacheMaximumSize(5000));

assertValidates(new BaseJdbcConfig()
.setConnectionUrl("jdbc:h2:mem:config")
.setMetadataCacheTtl(new Duration(1, SECONDS)));

assertFailsValidation(new BaseJdbcConfig()
.setCacheMaximumSize(5000),
"cacheMaximumSizeConsistent",
"metadata.cache-ttl must be set to a non-zero value when metadata.cache-maximum-size is set",
AssertTrue.class);

assertFailsValidation(new BaseJdbcConfig()
.setSchemaNamesCacheTtl(new Duration(1, SECONDS)),
"schemaNamesCacheTtlConsistent",
"metadata.schemas.cache-ttl must not be set when metadata.cache-ttl is not set",
AssertTrue.class);

assertFailsValidation(new BaseJdbcConfig()
.setTableNamesCacheTtl(new Duration(1, SECONDS)),
"tableNamesCacheTtlConsistent",
"metadata.tables.cache-ttl must not be set when metadata.cache-ttl is not set",
AssertTrue.class);
}

private static void buildConfig(Map<String, String> properties)
Expand Down
Loading