maytasm commented on code in PR #9280:
URL: https://github.com/apache/druid/pull/9280#discussion_r4318256510


##########
processing/src/main/java/org/apache/druid/query/CacheStrategy.java:
##########
@@ -48,21 +50,27 @@
   boolean isCacheable(QueryType query, boolean willMergeRunners);
 
   /**
-   * Computes the cache key for the given query
+   * Computes the per-segment cache key for the given query. Because this is a 
per-segment cache key, it should only
+   * include parts of the query that affect the results for a specific segment 
(i.e., the results returned from
+   * {@link QueryRunnerFactory#createRunner}).
    *
-   * @param query the query to compute a cache key for
+   * @param query the query to be cached
    *
-   * @return the cache key
+   * @return the per-segment cache key
    */
   byte[] computeCacheKey(QueryType query);
 
   /**
-   * Computes the result level cache key for the given query.
-   * Some implementations may include query parameters that might not be used 
in {@code computeCacheKey} for same query
+   * Computes the result-level cache key for the given query. The result-level 
cache will tack on datasource and

Review Comment:
   @gianm 
   
   > Fwiw, I think it would be good to modify ResultLevelCachingQueryRunner to 
make this part of the key, not part of the value, so it can cache results for 
queries that differ in interval but are the same otherwise. I don't have plans 
to do this personally though, at least not right now.
   
   Is there a reason the datasource and intervals were left out of the key, or 
does something depend on one entry per query shape? Adding the query interval 
to the ETag rather than the key, stopped different intervals from returning 
wrong results, but they still share one entry and overwrite each other. The one 
trade-off I can see: a query with a rolling "now" interval would add a new 
entry on every refresh instead of overwriting one. Those never hit today 
either, since the interval is in the ETag, so no hits would be lost. 
Size-bounded caches would see more churn, though.
   
   Some benefits with datasource and interval in the key:
   - **Metadata queries share one entry.** A metadata service polls datasources 
with an identical query shape, so they all land on one key and overwrite each 
other. 
   - **Alternating intervals never hit.** The same query run over intervals a, 
b, a, b gets zero hits today, because each run overwrites the other's entry. 
With intervals in the key, every repeat would hit as long as the segments don't 
change.



-- 
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]

Reply via email to