Skip to content

Ignore the queries in the future in the ingesters #1929

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
Merged
merged 1 commit into from
Mar 3, 2020
Merged

Ignore the queries in the future in the ingesters #1929

merged 1 commit into from
Mar 3, 2020

Conversation

gouthamve
Copy link
Contributor

@gouthamve gouthamve commented Dec 19, 2019 •
edited
Loading

We ignore queries in the future now.

Signed-off-by: Goutham Veeramachaneni [email protected]

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

cstyan
cstyan approved these changes Dec 19, 2019
View reviewed changes
gouthamve added a commit to gouthamve/cortex that referenced this pull request Dec 19, 2019
@gouthamve
Don't hit ingesters if the query is in the future.
1196358 
This is a temp workaround until
cortexproject#1929 is merged and
deployed.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve gouthamve mentioned this pull request Dec 19, 2019
Closed
codesome
codesome approved these changes Dec 20, 2019
View reviewed changes
pracucci
pracucci reviewed Dec 20, 2019
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.

Few questions, please:

  1. Why don't we enforce it in the querier, instead of duplicating the logic between the chunk store and ingester?
  2. Shouldn't we apply the same fix to ingester's Query() and v2Query()?
  3. Can we add unit tests, to avoid regressions?

@pracucci
Copy link
Contributor

Why don't we enforce it in the querier, instead of duplicating the logic between the chunk store and ingester?

I'm talking specifically about querier.NewQueryable() and querier.newUnifiedChunkQueryable(). To me looks more natural to validate the query time range at the beginning of the read path.

@bboreham
Copy link
Contributor

My sense is that the chunk store does it to avoid generating error messages about "missing tables" when the problem is in the query parameters.
I would agree that the querier is a better place to shortcut, but not sure if I would remove the protection entirely from the chunk store for the above reason.

@owen-d
Copy link
Contributor

owen-d commented Dec 20, 2019

What about wrapping the distributor queryable in order to short circuit these before they call the ingesters? Maybe here:
https://github.com/cortexproject/cortex/blob/master/pkg/querier/querier.go#L117

@gouthamve gouthamve force-pushed the ignore-future-queries-ingester branch 2 times, most recently from 866df34 to 414224f Compare January 15, 2020 20:16
cstyan
cstyan reviewed Jan 15, 2020
View reviewed changes
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

code lgtm, but I can't really comment on whether this is the right fix for what we were seeing

pracucci
pracucci previously approved these changes Jan 17, 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.

LGTM with few nits.

@gouthamve gouthamve force-pushed the ignore-future-queries-ingester branch 4 times, most recently from 95a7926 to 24f1ea2 Compare February 11, 2020 13:29
@gouthamve gouthamve force-pushed the ignore-future-queries-ingester branch from 24f1ea2 to ee9b67b Compare February 25, 2020 14:17
@gouthamve gouthamve requested a review from pracucci February 28, 2020 10:34
@gouthamve gouthamve dismissed pracucci’s stale review February 28, 2020 10:35

Lots of changes since. Owen's PR got merged.

pracucci
pracucci approved these changes Mar 2, 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.

LGTM! (just left a couple of nits)

CHANGELOG.md Outdated
Comment on lines 17 to 18
* [CHANGE] We now ignore queries that are more than 10mins into the future. #1929
* Can be configured using `querier.max-query-into-future`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it would be more clear:

Suggested change
* [CHANGE] We now ignore queries that are more than 10mins into the future. #1929
* Can be configured using `querier.max-query-into-future`.
* [CHANGE] We now enforce queries to be up to `-querier.max-query-into-future` into the future (defaults to 10m). #1929

Because we don't "ignore queries" but we actually clamp the max timestamp.

pkg/querier/querier_test.go Outdated
maxQueryIntoFuture: 10 * time.Minute,
},
{ // Skipping stores is disabled.
name: "hit-test2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Clashing name with the previous one. The name may be something like "max-query-into-future-disabled".

sandeepsukhani added a commit to grafana/cortex that referenced this pull request Mar 3, 2020
@sandeepsukhani
Merge PR cortexproject#1929 into r77
06a11ac
@gouthamve gouthamve force-pushed the ignore-future-queries-ingester branch from ee9b67b to b784a85 Compare March 3, 2020 16:54
@gouthamve gouthamve merged commit f0fb4b7 into cortexproject:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cstyan cstyan cstyan approved these changes

@pracucci pracucci pracucci approved these changes

@codesome codesome codesome 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.
6 participants
@gouthamve @pracucci @bboreham @owen-d @cstyan @codesome