Skip to content

Remove unused caches in BigQueryClient #10725

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
findepi merged 1 commit into from
Jan 21, 2022
Merged

Remove unused caches in BigQueryClient #10725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 0 additions & 5 deletions docs/src/main/sphinx/connector/bigquery.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,6 @@ Property Description
``bigquery.credentials-key`` The base64 encoded credentials key None. See the `requirements <#requirements>`_ section.
``bigquery.credentials-file`` The path to the JSON credentials file None. See the `requirements <#requirements>`_ section.
``bigquery.case-insensitive-name-matching`` Match dataset and table names case-insensitively ``false``
``bigquery.case-insensitive-name-matching.cache-ttl`` Duration for which remote dataset and table names will be ``1m``
cached. Higher values reduce the number of API calls to
BigQuery but can cause newly created dataset or tables to not
be visible until the configured duration. Set to ``0ms`` to
disable the cache.
===================================================== ============================================================== ======================================================

Data types
Expand Down
27 changes: 2 additions & 25 deletions plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
import com.google.cloud.bigquery.TableInfo;
import com.google.cloud.bigquery.TableResult;
import com.google.cloud.http.BaseHttpServiceException;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ImmutableSet;
import io.airlift.log.Logger;
import io.airlift.units.Duration;
Expand All @@ -54,7 +52,6 @@
import static java.lang.String.format;
import static java.util.Locale.ENGLISH;
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.stream.Collectors.joining;

public class BigQueryClient
Expand All @@ -64,21 +61,12 @@ public class BigQueryClient
private final BigQuery bigQuery;
private final ViewMaterializationCache materializationCache;
private final boolean caseInsensitiveNameMatching;
private final Cache<String, Optional<RemoteDatabaseObject>> remoteDatasets;
private final Cache<TableId, Optional<RemoteDatabaseObject>> remoteTables;

public BigQueryClient(BigQuery bigQuery, BigQueryConfig config, ViewMaterializationCache materializationCache)
{
this.bigQuery = requireNonNull(bigQuery, "bigQuery is null");
this.materializationCache = requireNonNull(materializationCache, "materializationCache is null");

Duration caseInsensitiveNameMatchingCacheTtl = requireNonNull(config.getCaseInsensitiveNameMatchingCacheTtl(), "caseInsensitiveNameMatchingCacheTtl is null");

this.caseInsensitiveNameMatching = config.isCaseInsensitiveNameMatching();
CacheBuilder<Object, Object> remoteNamesCacheBuilder = CacheBuilder.newBuilder()
.expireAfterWrite(caseInsensitiveNameMatchingCacheTtl.toMillis(), MILLISECONDS);
this.remoteDatasets = remoteNamesCacheBuilder.build();
this.remoteTables = remoteNamesCacheBuilder.build();
}

public Optional<RemoteDatabaseObject> toRemoteDataset(String projectId, String datasetName)
Expand All @@ -90,12 +78,6 @@ public Optional<RemoteDatabaseObject> toRemoteDataset(String projectId, String d
return Optional.of(RemoteDatabaseObject.of(datasetName));
}

Optional<RemoteDatabaseObject> remoteDataset = remoteDatasets.getIfPresent(datasetName);
if (remoteDataset != null) {
return remoteDataset;
}

// cache miss, reload the cache
Map<String, Optional<RemoteDatabaseObject>> mapping = new HashMap<>();
for (Dataset dataset : listDatasets(projectId)) {
mapping.merge(
Expand All @@ -104,8 +86,8 @@ public Optional<RemoteDatabaseObject> toRemoteDataset(String projectId, String d
(currentValue, collision) -> currentValue.map(current -> current.registerCollision(collision.get().getOnlyRemoteName())));
}

// explicitly cache the information if the requested dataset doesn't exist
if (!mapping.containsKey(datasetName)) {
// dataset doesn't exist
mapping.put(datasetName, Optional.empty());
}

Expand Down Expand Up @@ -134,12 +116,7 @@ private Optional<RemoteDatabaseObject> toRemoteTable(String projectId, String re
}

TableId cacheKey = TableId.of(projectId, remoteDatasetName, tableName);
Optional<RemoteDatabaseObject> remoteTable = remoteTables.getIfPresent(cacheKey);
if (remoteTable != null) {
return remoteTable;
}

// cache miss, reload the cache
Map<TableId, Optional<RemoteDatabaseObject>> mapping = new HashMap<>();
for (Table table : tables.get()) {
mapping.merge(
Expand All @@ -148,8 +125,8 @@ private Optional<RemoteDatabaseObject> toRemoteTable(String projectId, String re
(currentValue, collision) -> currentValue.map(current -> current.registerCollision(collision.get().getOnlyRemoteName())));
}

// explicitly cache the information if the requested table doesn't exist
if (!mapping.containsKey(cacheKey)) {
// table doesn't exist
mapping.put(cacheKey, Optional.empty());
}

Expand Down
18 changes: 2 additions & 16 deletions plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.airlift.configuration.Config;
import io.airlift.configuration.ConfigDescription;
import io.airlift.configuration.ConfigHidden;
import io.airlift.configuration.DefunctConfig;
import io.airlift.units.Duration;
import io.airlift.units.MinDuration;

Expand All @@ -27,6 +28,7 @@
import static java.util.concurrent.TimeUnit.HOURS;
import static java.util.concurrent.TimeUnit.MINUTES;

@DefunctConfig("bigquery.case-insensitive-name-matching.cache-ttl")
public class BigQueryConfig
{
public static final int DEFAULT_MAX_READ_ROWS_RETRIES = 3;
Expand All @@ -40,7 +42,6 @@ public class BigQueryConfig
private Optional<String> viewMaterializationDataset = Optional.empty();
private int maxReadRowsRetries = DEFAULT_MAX_READ_ROWS_RETRIES;
private boolean caseInsensitiveNameMatching;
private Duration caseInsensitiveNameMatchingCacheTtl = new Duration(1, MINUTES);
private Duration viewsCacheTtl = new Duration(15, MINUTES);
private Duration serviceCacheTtl = new Duration(3, MINUTES);

Expand Down Expand Up @@ -155,21 +156,6 @@ public BigQueryConfig setCaseInsensitiveNameMatching(boolean caseInsensitiveName
return this;
}

@NotNull
@MinDuration("0ms")
public Duration getCaseInsensitiveNameMatchingCacheTtl()
{
return caseInsensitiveNameMatchingCacheTtl;
}

@Config("bigquery.case-insensitive-name-matching.cache-ttl")
@ConfigDescription("Duration for which remote dataset and table names will be cached")
public BigQueryConfig setCaseInsensitiveNameMatchingCacheTtl(Duration caseInsensitiveNameMatchingCacheTtl)
{
this.caseInsensitiveNameMatchingCacheTtl = caseInsensitiveNameMatchingCacheTtl;
return this;
}

@NotNull
@MinDuration("0m")
public Duration getViewsCacheTtl()
Expand Down
4 changes: 0 additions & 4 deletions plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.MINUTES;
import static java.util.concurrent.TimeUnit.SECONDS;

public class TestBigQueryConfig
{
Expand All @@ -39,7 +38,6 @@ public void testDefaults()
.setViewMaterializationDataset(null)
.setMaxReadRowsRetries(3)
.setCaseInsensitiveNameMatching(false)
.setCaseInsensitiveNameMatchingCacheTtl(new Duration(1, MINUTES))
.setViewsCacheTtl(new Duration(15, MINUTES))
.setServiceCacheTtl(new Duration(3, MINUTES))
.setViewsEnabled(false));
Expand All @@ -57,7 +55,6 @@ public void testExplicitPropertyMappingsWithCredentialsKey()
.put("bigquery.view-materialization-dataset", "vmdataset")
.put("bigquery.max-read-rows-retries", "10")
.put("bigquery.case-insensitive-name-matching", "true")
.put("bigquery.case-insensitive-name-matching.cache-ttl", "1s")
.put("bigquery.views-cache-ttl", "1m")
.put("bigquery.service-cache-ttl", "10d")
.buildOrThrow();
Expand All @@ -71,7 +68,6 @@ public void testExplicitPropertyMappingsWithCredentialsKey()
.setViewMaterializationDataset("vmdataset")
.setMaxReadRowsRetries(10)
.setCaseInsensitiveNameMatching(true)
.setCaseInsensitiveNameMatchingCacheTtl(new Duration(1, SECONDS))
.setViewsCacheTtl(new Duration(1, MINUTES))
.setServiceCacheTtl(new Duration(10, DAYS));

Expand Down