pvary commented on code in PR #6111:
URL: https://github.com/apache/iceberg/pull/6111#discussion_r1021208714
##########
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:
Coming from a world where we were a platform for multiple companies, I would
chose the more conservative approach and try to avoid breaking changes unless
it is strictly necessary.
Also adding a new configuration which just provides another way to archive
the same function seems something which will lead to confusion down the line,
so I would opt for:
- keep the `cache-enabled` boolean flag
- if the `cache-enabled` is `true` find meaningful values for the
`cacheExpirationIntervalMs`, or throw an error message if they do not make
sense, so:
- if `cacheExpirationIntervalMs` is > 0 then expire the the element
- if `cacheExpirationIntervalMs` is <=0 then never expire the element
- if `cacheExpirationIntervalMs` is unset then never expire the element
to keep the backwards compatibility
- if the `cache-enabled` is `false` then ignore the
`cacheExpirationIntervalMs` or throw an exception if it is set.
Just my 2 cents
--
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
