Externalised destination table cache expiry duration for BigQuery Connector #8283
Conversation
| @@ -78,6 +80,10 @@ | |||
| .expireAfterWrite(caseInsensitiveNameMatchingCacheTtl.toMillis(), MILLISECONDS); | |||
| this.remoteDatasets = remoteNamesCacheBuilder.build(); | |||
| this.remoteTables = remoteNamesCacheBuilder.build(); | |||
| this.destinationTableCache = CacheBuilder.newBuilder() | |||
| .expireAfterWrite(config.getDestinationTableCacheExpiry().roundTo(TimeUnit.MINUTES), TimeUnit.MINUTES) | |||
There was a problem hiding this comment.
expireAfterWrite(config.getDestinationTableCacheExpiry().toMillis(), MILLISECONDS)
| return destinationTableCacheExpiry; | ||
| } | ||
|
|
||
| @Config("bigquery.destination-table-cache-expiry") |
There was a problem hiding this comment.
maybe bigquery.views-cache-ttl would be a better name
| } | ||
|
|
||
| @Config("bigquery.destination-table-cache-expiry") | ||
| @ConfigDescription("Duration for which query and destination table will be cached") |
There was a problem hiding this comment.
Duration for which the results of querying a view will be cached
| @@ -47,6 +48,7 @@ | |||
| private int maxReadRowsRetries = DEFAULT_MAX_READ_ROWS_RETRIES; | |||
| private boolean caseInsensitiveNameMatching; | |||
| private Duration caseInsensitiveNameMatchingCacheTtl = new Duration(1, MINUTES); | |||
| private Duration destinationTableCacheExpiry = new Duration(15, TimeUnit.MINUTES); | |||
| @@ -130,7 +122,7 @@ private TableInfo getActualTable( | |||
| String query = bigQueryClient.selectSql(table.getTableId(), requiredColumns); | |||
| log.debug("query is %s", query); | |||
| try { | |||
| return destinationTableCache.get(query, new DestinationTableBuilder(bigQueryClient, config, query, table.getTableId())); | |||
| return bigQueryClient.getDestinationTableCache().get(query, new DestinationTableBuilder(bigQueryClient, config, query, table.getTableId())); | |||
There was a problem hiding this comment.
I think we could move DestinationTableBuilder to BigQueryClient and expose a method bigQueryClient.getCachedTable(config, table.getTableId(), requiredColumns) instead of exposing the cache directly.
| @@ -129,76 +109,12 @@ private TableInfo getActualTable( | |||
| // get it from the view | |||
| String query = bigQueryClient.selectSql(table.getTableId(), requiredColumns); | |||
There was a problem hiding this comment.
I would move this into bigQueryClient.getCachedTable as well
| @@ -337,4 +362,63 @@ public boolean isAmbiguous() | |||
| return remoteNames.size() > 1; | |||
| } | |||
| } | |||
|
|
|||
| static class DestinationTableBuilder | |||
There was a problem hiding this comment.
Can the movement of this class be done in a preparatory first commit? It'll make the diff easier to understand that it's just a move and no code changed.
There was a problem hiding this comment.
Do you want me to make that change now? Or is it fine to go ahead?
There was a problem hiding this comment.
It would be better to have that since it makes it easier to make sense of history in the future.
So the move-only changes would get extracted to the first commit.
| TableInfo getCachedTable(ReadSessionCreatorConfig config, String query, TableInfo table) | ||
| { | ||
| try { | ||
| return destinationTableCache.get(query, |
There was a problem hiding this comment.
just noting that the cache key seems to be the entire query string (which can be large).
No changes requested at the moment though.
| @@ -13,6 +13,7 @@ | |||
| */ | |||
| package io.trino.plugin.bigquery; | |||
|
|
|||
There was a problem hiding this comment.
Please update commit message to Move DestinationTableBuilder to BigQueryClient
| @@ -44,6 +44,7 @@ | |||
| import java.util.Optional; | |||
There was a problem hiding this comment.
Please update commit message to Make BigQuery views cache ttl configurable
In general we try to follow https://chris.beams.io/posts/git-commit/ for commit messages
| @@ -47,6 +48,7 @@ | |||
| private int maxReadRowsRetries = DEFAULT_MAX_READ_ROWS_RETRIES; | |||
| private boolean caseInsensitiveNameMatching; | |||
| private Duration caseInsensitiveNameMatchingCacheTtl = new Duration(1, MINUTES); | |||
| private Duration viewsCacheTtl = new Duration(15, TimeUnit.MINUTES); | |||
There was a problem hiding this comment.
nit: MINUTES is already imported at L33. Please use it instead of TimeUnit.MINUTES.
There was a problem hiding this comment.
Ahh..my bad! I'll update.
|
Merged, thanks! |
Resolves #8236
Made destination table cache expiry duration configurable. This can be configured using bigquery.destination-table-cache-expiry property. Defaults to 15m.