Disable caching on brokers for groupBy v2#3950
Conversation
|
👍 |
| this.backgroundExecutorService = MoreExecutors.listeningDecorator(backgroundExecutorService); | ||
|
|
||
| if (cacheConfig.isQueryCacheable(Query.GROUP_BY)) { | ||
| log.warn("Caching for groupBys is enabled, but they won't be cached if they are executed with the strategy v2!"); |
There was a problem hiding this comment.
can you also clarify that they won't be cached on druid brokers also an advise to enable caching on data nodes for V2 strategy would be helpful.
There was a problem hiding this comment.
How about this wording:
"Even though groupBy caching is enabled, v2 groupBys will not be cached. Consider disabling cache on your broker and enabling it on your data nodes to enable v2 groupBy caching."
|
@jihoonson: left a minor comment about log message, LGTM otherwise. |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Some suggested restructuring -- looks good in general though
| this.backgroundExecutorService = MoreExecutors.listeningDecorator(backgroundExecutorService); | ||
|
|
||
| if (cacheConfig.isQueryCacheable(Query.GROUP_BY)) { | ||
| log.warn("Caching for groupBys is enabled, but they won't be cached if they are executed with the strategy v2!"); |
There was a problem hiding this comment.
How about this wording:
"Even though groupBy caching is enabled, v2 groupBys will not be cached. Consider disabling cache on your broker and enabling it on your data nodes to enable v2 groupBy caching."
| * The {@code willMergeRunners} parameter can be used for distinguishing the caller is a broker or a data node. | ||
| * | ||
| * @param query the query to be cached | ||
| * @param willMergeRunners indicates that {@link QueryRunnerFactory#mergeRunners(ExecutorService, Iterable)} will be |
There was a problem hiding this comment.
For clarification add, "will be called on the cached by-segment results"
| @Override | ||
| public boolean isCacheable(GroupByQuery query, boolean willMergeRunners) | ||
| { | ||
| return willMergeRunners || !strategySelector.useStrategyV2(query); |
There was a problem hiding this comment.
IMO, this should be part of the strategy, since that's the point of the strategies. So the code could be:
return strategySelector.strategize(query).isCacheable(willMergeRunners);Along with an "isCacheable" method added to the strategies.
|
@nishantmonu51 and @gianm thanks for your review. I addressed comments. |
4 participants
This PR fixes #3820.
This change is