Akshat-Jain commented on code in PR #17212:
URL: https://github.com/apache/druid/pull/17212#discussion_r1799925128
##########
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:
@TessaIO Thanks for the details!
I also tried it locally for a query like:
```sql
select name, LOOKUP(name, 'singlelookup1') from "druid"."test-ds-1"
```
such that there exists a `name` in `test-ds-1` which doesn't exist in the
JDBC source that backs `singlelookup1`.
I got the following error:
```
CacheLoader returned null for key somename.
com.google.common.cache.CacheLoader$InvalidCacheLoadException
```
The root cause seems to be that `loadingCache.get(keyEquivalent, new
ApplyCallable(keyEquivalent))` doesn't work if the callable returns null, as
the underlying cache implementations (guava and mapDB) don't support loading
null values. Guava returns the above error message, whereas mapDB returned some
ISE.
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.
I was wondering about alternate approaches, like continuing to use
`loadingCache.get()` and maybe checking the thrown exceptions and returning
null manually from `LoadingLookup#apply`. But it's also not ideal since the
exception messages differ across Guava and MapDB implementations.
Do you have any further thoughts on the above?
@abhishekrb19 Hey! Would appreciate your inputs as well on the above. Thanks!
--
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]
