vlsi commented on a change in pull request #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401480724
##########
File path:
core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique
will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
The best practice is:
1) Have sensible defaults. I agree that making the cache with a limit of 128
by default is good.
2) Provide a way to deactivate the feature. There might be cases where the
feature introduces regressions. E.g. memory leaks. Then, a flag to disable the
feature will help until the bug is resolved. For instance, `maximum.size=0`
might mean `cache is disabled`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
