[GitHub] [iceberg] pvary commented on a diff in pull request #6111: Flink: Add 'cache.expiration-interval-ms' option to FlinkCatalog

[GitBox <gi...@apache.org>]

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-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org