findepi commented on code in PR #3543:
URL: https://github.com/apache/iceberg/pull/3543#discussion_r1650929394
##########
core/src/main/java/org/apache/iceberg/CachingCatalog.java:
##########
@@ -29,24 +34,91 @@
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
-
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class that wraps an Iceberg Catalog to cache tables.
+ * <p>
+ * See {@link CatalogProperties#CACHE_EXPIRATION_INTERVAL_MS} for more details
+ * regarding special values for {@code expirationIntervalMillis}.
+ */
public class CachingCatalog implements Catalog {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(CachingCatalog.class);
+ private static final RemovalListener<TableIdentifier, Table>
identLoggingRemovalListener =
+ (key, value, cause) -> LOG.debug("Evicted {} from the table cache ({})",
key, cause);
+
public static Catalog wrap(Catalog catalog) {
- return wrap(catalog, true);
+ return wrap(catalog, CatalogProperties.CACHE_EXPIRATION_INTERVAL_MS_OFF);
+ }
+
+ public static Catalog wrap(Catalog catalog, long expirationIntervalMillis) {
+ return wrap(catalog, true, expirationIntervalMillis);
}
- public static Catalog wrap(Catalog catalog, boolean caseSensitive) {
- return new CachingCatalog(catalog, caseSensitive);
+ public static Catalog wrap(Catalog catalog, boolean caseSensitive, long
expirationIntervalMillis) {
+ return new CachingCatalog(catalog, caseSensitive,
expirationIntervalMillis);
}
- private final Cache<TableIdentifier, Table> tableCache =
Caffeine.newBuilder().softValues().build();
private final Catalog catalog;
private final boolean caseSensitive;
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected final long expirationIntervalMillis;
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected final Cache<TableIdentifier, Table> tableCache;
+
+ private CachingCatalog(Catalog catalog, boolean caseSensitive, long
expirationIntervalMillis) {
+ this(catalog, caseSensitive, expirationIntervalMillis,
Ticker.systemTicker());
+ }
- private CachingCatalog(Catalog catalog, boolean caseSensitive) {
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected CachingCatalog(Catalog catalog, boolean caseSensitive, long
expirationIntervalMillis, Ticker ticker) {
+ Preconditions.checkArgument(expirationIntervalMillis != 0,
+ "When %s is set to 0, the catalog cache should be disabled. This
indicates a bug.",
+ CatalogProperties.CACHE_EXPIRATION_INTERVAL_MS);
this.catalog = catalog;
this.caseSensitive = caseSensitive;
+ this.expirationIntervalMillis = expirationIntervalMillis;
+ this.tableCache = createTableCache(ticker);
+ }
+
+ /**
+ * CacheWriter class for removing metadata tables when their associated data
table is expired
+ * via cache expiration.
+ */
+ class MetadataTableInvalidatingCacheWriter implements
CacheWriter<TableIdentifier, Table> {
+ @Override
+ public void write(TableIdentifier tableIdentifier, Table table) {
+ }
+
+ @Override
+ public void delete(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
+ if (RemovalCause.EXPIRED.equals(cause)) {
+ if (!MetadataTableUtils.hasMetadataTableName(tableIdentifier)) {
+ tableCache.invalidateAll(metadataTableIdentifiers(tableIdentifier));
+ }
Review Comment:
This obviously races with hash puts done in
`org.apache.iceberg.CachingCatalog#loadTable`.
Would be great to have a code comment instructing the reader why it's not a
problem.
Is the thinking that this is not required for correctness and therefore does
not have to be 100% accurate?
--
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]
