Skip to content

Clarify that Broker caching for groupBy v2 queries does not work#11370

Merged
sthetland merged 3 commits intoapache:masterfrom
FrankChen021:groupv2_cache
Aug 3, 2021
Merged

Clarify that Broker caching for groupBy v2 queries does not work#11370
sthetland merged 3 commits intoapache:masterfrom
FrankChen021:groupv2_cache

Conversation

@FrankChen021
Copy link
Copy Markdown
Member

Description

Broker caching has been disabled for groupBy v2 queries since #3950, and this limitation is stated under the section Differences between v1 and v2 of groupBy query doc. But since that's a large block of text, it's not intuitive for users to notice this constraint.

This PR adds a note under the section Broker Caching of the configuration doc to give users a more clear hint when they follow that doc to enable broker caching.

This PR has:

  • been self-reviewed.

Copy link
Copy Markdown

@sthetland sthetland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to make this more prominent. Just one suggestion to put the groupBy term into its usual form.

Comment thread docs/configuration/index.md Outdated
Co-authored-by: sthetland <steve.hetland@imply.io>
@FrankChen021 FrankChen021 requested a review from sthetland July 2, 2021 02:09
@capistrant
Copy link
Copy Markdown
Contributor

is groupBy v2 only not cacheable for the non-result cache or is it also not cacheable for result cache? If it is only for non-result cache, I think we should explicitly call that out to not confuse people.

@FrankChen021
Copy link
Copy Markdown
Member Author

@capistrant Both of per-segment caching and whole-query caching are not supported for groupBy v2 queries. It's a good idea to put this in the doc to make it more clear.

Copy link
Copy Markdown
Contributor

@capistrant capistrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. the trick will be for someone to remember to remove this from the docs if down the road some day someone implements broker caching for groupby v2

@FrankChen021
Copy link
Copy Markdown
Member Author

Hi @sthetland , Do you have any other comments ?

@sthetland sthetland merged commit 55a01a0 into apache:master Aug 3, 2021
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
@FrankChen021 FrankChen021 deleted the groupv2_cache branch August 13, 2021 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants