magibney commented on code in PR #1481:
URL: https://github.com/apache/solr/pull/1481#discussion_r1145441657
##########
solr/core/src/java/org/apache/solr/search/CaffeineCache.java:
##########
@@ -250,26 +249,35 @@ public V computeIfAbsent(K key, IOFunction<? super K, ?
extends V> mappingFuncti
return computeAsync(key, mappingFunction);
}
- try {
- return cache.get(
- key,
- k -> {
- V value;
- try {
- value = mappingFunction.apply(k);
- } catch (IOException e) {
- throw new UncheckedIOException(e);
- }
- if (value == null) {
- return null;
- }
- recordRamBytes(key, null, value);
- inserts.increment();
- return value;
- });
- } catch (UncheckedIOException e) {
- throw e.getCause();
+ /*
+ Synchronous caches must effectively use get-then-put under the hood in
place of
+ `computeIfAbsent()`-type behavior. A number of Solr queries (and
consequently,
+ `mappingFunction`s passed into this method) would modify the cache
recursively.
+ This is supported in `async` mode, but when `async==false`, it may yield
+ "IllegalStateException: Recursive update" (see SOLR-16707).
+ */
+ V cached = cache.getIfPresent(key);
+ if (cached != null) {
+ return cached;
+ }
+ final V computed = mappingFunction.apply(key);
Review Comment:
Oh 100%, no doubt. In fact I was pretty clear about being in favor of
removing the possibility of non-async in [comments on the related
issue](https://issues.apache.org/jira/browse/SOLR-16707?focusedCommentId=17703668#comment-17703668).
But until we can deprecate/remove that option, we have two options to
continue to support `async=false`, both not great:
1. document that users need to avoid join queries, avoid certain
configurations of range queries and boolean queries, and ?? maybe other
things?, or
2. revert the behavior of `async=false` to what it would have been
pre-SOLR-15555, and have everything work, albeit with possible
double-computation.
This PR goes the second route. Which, faced with the tradeoff that route
implies for `async=false`, makes one wonder why anyone would ever configure
`async=false`? But then we're [back to the
question](https://issues.apache.org/jira/browse/SOLR-16707?focusedCommentId=17702769#comment-17702769)
of whether there's any benefit to keeping `async=false` as an option at all.
In the meantime, this PR will make things work for users who (for whatever
reason) enable this option, and it will also stop the unhelpful test failures.
--
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...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
