jtuglu1 commented on code in PR #20815:
URL: https://github.com/apache/druid/pull/20815#discussion_r4504360086
##########
server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java:
##########
@@ -95,7 +98,7 @@ public Sequence<T> run(QueryPlus queryPlus, ResponseContext
responseContext)
{
if (useResultCache || populateResultCache) {
- final byte[] queryCacheKey = strategy.computeResultLevelCacheKey(query);
+ final byte[] queryCacheKey = computeResultLevelCacheKey();
final Cache.NamedKey cacheKey =
CacheUtil.computeResultLevelCacheKey(queryCacheKey);
final byte[] cachedResultSet =
fetchResultsFromResultLevelCache(cacheKey);
Review Comment:
something I noticed while looking over
[this](https://github.com/maytasm/druid/blob/f577780e1c7d044cddda8e629c3e8a1dadbae678/server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java#L366)
code is that looks like there might? be a further way to increase hit rate.
Consider 3 query instances that map to the same query cache key:
```
q1: read cache, calculate E1 from old segments set, start execution
q2: read cache, calculate E2 from new segments set, start execution
q2: finish, write (key, {E2, results2})
q1: finish, write (key, {E1, results1})
q3: read cached {E1, results1}
q3: calculate E2 from current segments set
q3: reject E1, execute query, write (key, {E2, results3})
```
In order to prevent q3 from re-executing, we might be able to either:
- Do some sort of cache.putIfAbsentOrOlder() which sets the result if it is
not there yet or it is there AND the etag represents a segment set that is
"fresher" than the prior cached result.
- Alternatively, can maybe think about storing the etag in the cache key
itself which would blow up the cache size potentially (and you'd need eviction
policy which is complicated)
##########
server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java:
##########
@@ -95,7 +98,7 @@ public Sequence<T> run(QueryPlus queryPlus, ResponseContext
responseContext)
{
if (useResultCache || populateResultCache) {
- final byte[] queryCacheKey = strategy.computeResultLevelCacheKey(query);
+ final byte[] queryCacheKey = computeResultLevelCacheKey();
final Cache.NamedKey cacheKey =
CacheUtil.computeResultLevelCacheKey(queryCacheKey);
final byte[] cachedResultSet =
fetchResultsFromResultLevelCache(cacheKey);
Review Comment:
something I noticed while looking over
[this](https://github.com/maytasm/druid/blob/f577780e1c7d044cddda8e629c3e8a1dadbae678/server/src/main/java/org/apache/druid/query/ResultLevelCachingQueryRunner.java#L366)
code is that looks like there might? be a further way to increase hit rate.
Consider 3 query instances that map to the same query cache key:
```
q1: read cache, calculate E1 from old segments set, start execution
q2: read cache, calculate E2 from new segments set, start execution
q2: finish, write (key, {E2, results2})
q1: finish, write (key, {E1, results1})
q3: read cached {E1, results1}
q3: calculate E2 from current segments set (same as q2)
q3: reject E1, execute query, write (key, {E2, results3})
```
In order to prevent q3 from re-executing, we might be able to either:
- Do some sort of cache.putIfAbsentOrOlder() which sets the result if it is
not there yet or it is there AND the etag represents a segment set that is
"fresher" than the prior cached result.
- Alternatively, can maybe think about storing the etag in the cache key
itself which would blow up the cache size potentially (and you'd need eviction
policy which is complicated)
--
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]
