abhishekrb19 commented on code in PR #17212:
URL: https://github.com/apache/druid/pull/17212#discussion_r1800491328
##########
extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java:
##########
@@ -74,14 +75,22 @@ public String apply(@Nullable final String key)
}
final String presentVal;
- try {
- presentVal = loadingCache.get(keyEquivalent, new
ApplyCallable(keyEquivalent));
+ presentVal = this.loadingCache.getIfPresent(keyEquivalent);
+ if (presentVal != null) {
return NullHandling.emptyToNullIfNeeded(presentVal);
}
- catch (ExecutionException e) {
- LOGGER.debug("value not found for key [%s]", key);
+
+ String val = this.dataFetcher.fetch(keyEquivalent);
+ if (val == null) {
return null;
}
+
+ Map<String, String> map = new HashMap<>();
+ map.put(keyEquivalent, val);
+
+ this.loadingCache.putAll(map);
+
+ return NullHandling.emptyToNullIfNeeded(val);
Review Comment:
Tl;dr: The approach in this latest patch looks okay to me.
1. If we use `getIfPresent()` and not the `ApplyCallable` approach, we
should remove the unused `ApplyCallable` class
2. Add a test to ensure fetching a non-existent key from the data fetcher
works as expected - does the `testApplyWithNullCase()` case already cover that?
-----
**Alternative idea:**
@Akshat-Jain, re this:
> While the above query does work with your code changes, we would lose out
on the "collection of cache statistics" part which was happening in the
loadingCache.get() flow.
Are you referring to the off heap/on heap stats? I see for the off heap
loading cache, the stats are in fact only modified in the `getIfPresent()`
[call](https://github.com/apache/druid/blob/master/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/loading/OffHeapLoadingCache.java#L113).
Not sure why those stats are not adjusted in `get()` call, which seems like a
separate bug to me.
A similar approach to what Akshat is referring to could also work by
throwing an exception. Fwiw, this is called out in the
[javadocs](https://github.com/apache/druid/blob/master/extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/cache/loading/LoadingCache.java#L74)
for `LoadingCache#get(K, Callable)`:
```
* <p><b>Warning:</b> as with {@link CacheLoader#load}, {@code
valueLoader} <b>must not</b> return
* {@code null}; it may either return a non-null value or throw an
exception.
```
So it looks throwing an exception is another way to handle this if we go
down the `get(K, Callable)`. So we can perhaps change `ApplyCallable#call()` to
explicitly check for null value and throw an exception, but it must be caught
and handled in `apply()` to return null. I don't necessarily like this approach
because it relies on throwing and handling checked exceptions just to work
around the underlying library's implementation.
--
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]
