gaborkaszab commented on code in PR #14398:
URL: https://github.com/apache/iceberg/pull/14398#discussion_r2695404793
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -165,6 +172,38 @@ public class RESTSessionCatalog extends
BaseViewSessionCatalog
private Supplier<Map<String, String>> mutationHeaders = Map::of;
private String namespaceSeparator = null;
+ @VisibleForTesting
+ @Value.Immutable
+ abstract static class SessionIdTableId {
+ public abstract String sessionId();
+
+ public abstract TableIdentifier tableIdentifier();
+
+ public static SessionIdTableId of(String sessionId, TableIdentifier ident)
{
+ return ImmutableSessionIdTableId.builder()
+ .sessionId(sessionId)
+ .tableIdentifier(ident)
+ .build();
+ }
+ }
+
+ @VisibleForTesting
+ @Value.Immutable
+ abstract static class TableSupplierWithETag {
Review Comment:
Done
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3475,6 +3535,496 @@ public void
testLoadTableWithMissingMetadataFile(@TempDir Path tempDir) {
.hasMessageContaining("No in-memory file found for location: " +
metadataFileLocation);
}
+ @Test
+ public void testInvalidTableCacheParameters() {
Review Comment:
Let me create a separate PR to move whatever we have into a separate test
suite, introducing the framework there. I'll put this PR on top of that just to
not increase this one further.
##########
core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java:
##########
@@ -43,6 +45,14 @@ private RESTCatalogProperties() {}
public static final String REST_SCAN_PLAN_ID = "rest-scan-plan-id";
+ // Properties that control the behaviour of the table cache used for
freshness-aware table
+ // loading.
+ public static final String TABLE_CACHE_EXPIRE_AFTER_WRITE_MS =
"rest-table-cache-expire-after-ms";
Review Comment:
I went for `expire-after-write`. It expresses more the intent.
Additionally, I saw that in catalog properties there is a dot after `cache`
so I followed that pattern.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -470,38 +599,53 @@ public Table loadTable(SessionContext context,
TableIdentifier identifier) {
tableMetadata = response.tableMetadata();
}
+ List<Credential> credentials = response.credentials();
RESTClient tableClient = client.withAuthSession(tableSession);
+ Supplier<BaseTable> tableSupplier =
+ () ->
+ createTableSupplier(
+ finalIdentifier, tableMetadata, context, tableClient,
tableConf, credentials);
+
+ String eTag = responseHeaders.getOrDefault(HttpHeaders.ETAG, null);
+ if (eTag != null) {
+ tableCache.put(
+ SessionIdTableId.of(context.sessionId(), finalIdentifier),
+ TableSupplierWithETag.of(tableSupplier, eTag));
+ }
+
+ if (metadataType != null) {
+ return
MetadataTableUtils.createMetadataTableInstance(tableSupplier.get(),
metadataType);
+ }
+
+ return tableSupplier.get();
+ }
+
+ private BaseTable createTableSupplier(
Review Comment:
I changed this to return a supplier
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3121,6 +3126,7 @@ public void testCommitStateUnknownNotReconciled() {
.satisfies(ex -> assertThat(((CommitStateUnknownException)
ex).getSuppressed()).isEmpty());
}
+ @SuppressWarnings("MethodLength")
Review Comment:
Note to myself: This is probably some leftover, can be removed
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -277,9 +316,44 @@ public void initialize(String name, Map<String, String>
unresolved) {
mergedProps,
RESTCatalogProperties.REST_SCAN_PLANNING_ENABLED,
RESTCatalogProperties.REST_SCAN_PLANNING_ENABLED_DEFAULT);
+
+ this.tableCache = tableCacheBuilder(mergedProps).build();
+
super.initialize(name, mergedProps);
}
+ protected Caffeine<Object, Object> tableCacheBuilder(Map<String, String>
props) {
+ long expireAfterWriteMS =
+ PropertyUtil.propertyAsLong(
+ props,
+ RESTCatalogProperties.TABLE_CACHE_EXPIRE_AFTER_WRITE_MS,
+ RESTCatalogProperties.TABLE_CACHE_EXPIRE_AFTER_WRITE_MS_DEFAULT);
+ Preconditions.checkArgument(
+ expireAfterWriteMS > 0, "Invalid expire after write: zero or
negative");
+
+ long numEntries =
+ PropertyUtil.propertyAsLong(
+ props,
+ RESTCatalogProperties.TABLE_CACHE_MAX_ENTRIES,
+ RESTCatalogProperties.TABLE_CACHE_MAX_ENTRIES_DEFAULT);
+ Preconditions.checkArgument(numEntries >= 0, "Invalid max entries:
negative");
+
+ Caffeine<Object, Object> builder =
Review Comment:
With the RESTTableClass I managed to get rid of this extra step of returning
a builder here so that I can inject a Ticker in the Testable class. I could
solve this with a dedicated constructor for tests similarly as in
CachingCatalog.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -165,6 +172,38 @@ public class RESTSessionCatalog extends
BaseViewSessionCatalog
private Supplier<Map<String, String>> mutationHeaders = Map::of;
private String namespaceSeparator = null;
+ @VisibleForTesting
+ @Value.Immutable
+ abstract static class SessionIdTableId {
Review Comment:
Initially I started with that and found that not much code got into that
class other than delegating stuff to the underlying cache. I created
RESTTableCache now and apparently makes sense.
On downside is that this design introduces another level of indirection for
the tests that want to check the internal state or stats of the Caffeine cache,
but this is something I can live with.
Compared to FileIOTracker, I made RESTTableCache package private and not
public as I don't really want to expose this further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
