lvyanquan commented on code in PR #6111:
URL: https://github.com/apache/iceberg/pull/6111#discussion_r1021333953
##########
flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalogFactory.java:
##########
@@ -145,8 +145,27 @@ protected Catalog createCatalog(
baseNamespace =
Namespace.of(properties.get(BASE_NAMESPACE).split("\\."));
}
- boolean cacheEnabled =
Boolean.parseBoolean(properties.getOrDefault(CACHE_ENABLED, "true"));
- return new FlinkCatalog(name, defaultDatabase, baseNamespace,
catalogLoader, cacheEnabled);
+ boolean cacheEnabled = CatalogProperties.CACHE_ENABLED_DEFAULT;
+ if (properties.containsKey(CatalogProperties.CACHE_ENABLED)) {
+ cacheEnabled =
Boolean.parseBoolean(properties.get(CatalogProperties.CACHE_ENABLED));
+ }
+
+ long cacheExpirationIntervalMs =
+ PropertyUtil.propertyAsLong(
+ properties,
+ CatalogProperties.CACHE_EXPIRATION_INTERVAL_MS,
+ CatalogProperties.CACHE_EXPIRATION_INTERVAL_MS_DEFAULT);
+ if (cacheExpirationIntervalMs == 0) {
Review Comment:
Addressed for these suggestions:
> * if `cacheExpirationIntervalMs` is unset then never expire the element
to keep the backwards compatibility.
1. Use CatalogProperties.CACHE_EXPIRATION_INTERVAL_MS_OFF as default value
to keep the same behavior.
> * if <=0, caching never expire automatically. this is the difference.
don'y disable caching with value 0 because boolean flag serves the purpose
already.
2. Howerver, 0 value is excluded in the constructor of
[CachingCatalog](https://github.com/apache/iceberg/blob/dbb8a404f6632a55acb36e949f0e7b84b643cede/core/src/main/java/org/apache/iceberg/CachingCatalog.java)
as the following codeļ¼
```
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);
}
```
So cacheExpirationIntervalMs is not allowed to be 0, add this restriction to
Precondition and Document too.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
