rdblue commented on a change in pull request #3543:
URL: https://github.com/apache/iceberg/pull/3543#discussion_r766265684
##########
File path: core/src/main/java/org/apache/iceberg/CachingCatalog.java
##########
@@ -30,23 +35,98 @@
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
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 table's.
+ *
+ * When `expirationIntervalMillis` is non-zero, the cache will expire tables
after
+ * the given interval provided they have not been accessed via the catalog in
that time.
+ * Each lookup of the table via the catalog will restart the expiration time.
+ *
+ * If the duration is zero, the cache will retain entries forever, or until
+ * an action is taken on the cache to refresh the cache's table entry.
+ */
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("{} was evicted from the TableCache
with {} cause", key, cause);
+
+ // TODO - Update this to use the default expiration interval millis once
things are more complete.
+ // Don't want to break current tests.
public static Catalog wrap(Catalog catalog) {
- return wrap(catalog, true);
+ return wrap(catalog, 0);
+ }
+
+ public static Catalog wrap(Catalog catalog, long expirationIntervalMillis) {
+ return wrap(catalog, true, expirationIntervalMillis);
+ }
+
+ public static Catalog wrap(Catalog catalog, Duration expirationInterval) {
+ return wrap(catalog, true, expirationInterval.toMillis());
}
- 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;
+ private final boolean expirationEnabled;
+ @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) {
this.catalog = catalog;
this.caseSensitive = caseSensitive;
+ // Set negative values to zero to avoid creating a Duration with a
negative value
+ this.expirationIntervalMillis = expirationIntervalMillis <= 0 ? 0 :
expirationIntervalMillis;
+ this.expirationEnabled = expirationIntervalMillis > 0;
+ this.tableCache = createTableCache(ticker);
+ }
+
+ private Cache<TableIdentifier, Table> createTableCache(Ticker ticker) {
+ Caffeine<TableIdentifier, Table> cacheBuilder = Caffeine
+ .newBuilder()
+ .softValues()
+ .removalListener(identLoggingRemovalListener);
+
+ // Only setup ticker and background cleanup tasks from expiration if table
expiration is enabled.
+ if (expirationEnabled) {
+ return cacheBuilder
+ .writer(new CacheWriter<TableIdentifier, Table>() {
+ @Override
+ public void write(TableIdentifier tableIdentifier, Table table) {
+ }
+
+ @Override
+ public void delete(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
+ // On expiration (e.g. eviction due to `expireAfterAccess` or
`expireAfterWrite` policies),
+ // perform any necessary cache cleanup so that subsequent
catalog loads won't
+ // return stale metadata tables w.r.t. the underlying data
tables they would return.
+ //
+ // TODO: Consider moving all table expiration here, including
explicit calls to `invalidate`.
+ // This call is asynchornous in a background thread though.
Review comment:
I think you can remove the TODO. You're right to think about whether we
should do that if it is async, and I don't think it is a good idea. If you
invalidate an entry, then I like removing all metadata tables synchronously.
--
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]
