moomindani commented on code in PR #14440:
URL: https://github.com/apache/iceberg/pull/14440#discussion_r3745689634
##########
core/src/main/java/org/apache/iceberg/CachingCatalog.java:
##########
@@ -66,23 +108,61 @@ public static Catalog wrap(
@SuppressWarnings("checkstyle:VisibilityModifier")
protected final long expirationIntervalMillis;
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected long expireAfterWriteIntervalMillis;
+
@SuppressWarnings("checkstyle:VisibilityModifier")
Review Comment:
nit: this is @gaborkaszab's [earlier
comment](https://github.com/apache/iceberg/pull/14440#discussion_r2594640819)
still open — the `#` prefix isn't needed here, `{@link CachingCatalog(Catalog,
boolean, long, Ticker, long)}` works. Non-blocking, but it'd be good to close
out the thread since it's been open a while.
##########
core/src/main/java/org/apache/iceberg/CatalogProperties.java:
##########
@@ -49,22 +49,27 @@ private CatalogProperties() {}
public static final boolean CACHE_CASE_SENSITIVE_DEFAULT = true;
/**
- * Controls the duration for which entries in the catalog are cached.
+ * Controls Iceberg cache behavior
*
- * <p>Behavior of specific values of cache.expiration-interval-ms:
+ * <p>Cache expiration is controlled by two configurable policies that can
work independently or
+ * together:
*
* <ul>
- * <li>Zero - Caching and cache expiration are both disabled
- * <li>Negative Values - Cache expiration is turned off and entries expire
only on refresh etc
- * <li>Positive Values - Cache entries expire if not accessed via the
cache after this many
- * milliseconds
+ * <li>{@link CatalogProperties#CACHE_EXPIRATION_INTERVAL_MS} - Controls
expire-after-access
+ * expiration policy
+ * <li>{@link CatalogProperties#CACHE_EXPIRATION_AFTER_WRITE_INTERVAL_MS}
- Controls
+ * expire-after-write expiration policy
* </ul>
*/
public static final String CACHE_EXPIRATION_INTERVAL_MS =
"cache.expiration-interval-ms";
public static final long CACHE_EXPIRATION_INTERVAL_MS_DEFAULT =
TimeUnit.SECONDS.toMillis(30);
public static final long CACHE_EXPIRATION_INTERVAL_MS_OFF = -1;
Review Comment:
Could this get a javadoc? Every other property in this class documents its
semantics, and the value ranges matter here: `0` disables expire-after-write
(the default), and it interacts with `cache.expiration-interval-ms` — worth
stating which one wins and what a positive value means relative to
`expireAfterAccess`. Non-blocking for the approval, but it's the kind of thing
users hit first.
--
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]
