Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate cache interval #2040

Merged
merged 9 commits into from
Jan 30, 2020
Merged

Deprecate cache interval #2040

merged 9 commits into from
Jan 30, 2020

Conversation

owen-d
Copy link
Contributor

@owen-d owen-d commented Jan 27, 2020

What

This PR deprecates frontend.cache-split-interval in favor of using querier.split-queries-by-interval, reducing the configuration burden. Additionally, these two configurations should be in alignment when both splitting and caching middlewares are enabled, which is now ensured. In order to not cause compatibility issues, it will continue to use the old frontend.cache-split-interval when specified, but log a deprecation message.

Issue

closes #2038

cyriltovena
cyriltovena approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

pracucci
pracucci reviewed Jan 28, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

@owen-d do you mind rebasing master, please? We've merge the release 0.6.0 and you would need to move your CHANGELOG.md entry to the top of the file (remember to keep entries grouped by type).

pkg/querier/queryrange/results_cache.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/querier/queryrange/results_cache.go Show resolved Hide resolved
@owen-d owen-d force-pushed the deprecate-cache-interval branch from 50b497d to c8e31e1 Compare January 28, 2020 16:02
pracucci
pracucci reviewed Jan 28, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
owen-d added 4 commits January 28, 2020 14:33
@owen-d
deprecates frontend.cache-split-interval in favor of querier.split-qu…
1dbcc05 
…eries-by-interval

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d
regens docs, updates changelog
ac8634f 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d
adds PR #
64932d2 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d
removes frontend.cache-split-interval
4e8794d 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d force-pushed the deprecate-cache-interval branch from b897813 to 4e8794d Compare January 28, 2020 19:34
@owen-d
regens docs
bba96ab 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d force-pushed the deprecate-cache-interval branch from 1e877c2 to bba96ab Compare January 28, 2020 20:57
pracucci
pracucci reviewed Jan 29, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @owen-d for patiently addressing my feedback! The new design LGTM. I just left a couple of minor comments before approving.

pkg/querier/queryrange/results_cache.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/roundtrip.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/roundtrip.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@owen-d
addresses pr comments, config no longer satisfies CacheSplitter ifc
6d3e98b 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
jtlisi
jtlisi approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

I added some minor suggestions.

pkg/querier/queryrange/results_cache_test.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/results_cache_test.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/results_cache_test.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/roundtrip.go Show resolved Hide resolved
pracucci
pracucci approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @owen-d for addressing my feedback. LGTM. Once the nits are solved, we'll merge it.

pkg/querier/queryrange/results_cache_test.go Show resolved Hide resolved
owen-d added 3 commits January 30, 2020 08:54
@owen-d
passes logger to config validation to log warning/mutation
33894b2 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d
uses day const in tests
cd85bec 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d
consistent renaming for constSplitter tests
9c69da2 
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@jtlisi jtlisi merged commit 1462d45 into cortexproject:master Jan 30, 2020
@owen-d owen-d mentioned this pull request Jan 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyriltovena cyriltovena cyriltovena approved these changes

@jtlisi jtlisi jtlisi approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

frontend.cache-split-interval is unnecessary and adds misconfiguration pitfall.
4 participants
@owen-d @cyriltovena @pracucci @jtlisi