stevenzwu commented on code in PR #6111:
URL: https://github.com/apache/iceberg/pull/6111#discussion_r1022200851
##########
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:
latest code looks good to me. Will merge tomorrow unless there are other
comments.
We are on the same page regarding backward compatibility for keeping the
boolean flag.
> cacheExpirationIntervalMs is not allowed to be 0
agree with the Precondition. on the other hand, I think it is is a bit
unnecessary for the CachingCatalog. it could have treated `<=0` as no
expiration.
--
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]
