-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
feat(kafka): Enable querier to optionally query partition ingesters #14418
Conversation
pkg/querier/ingester_querier.go
Outdated
return nil, err | ||
} | ||
tenantShards := q.getShardCountForTenant(tenantID) | ||
// TODO: Confirm the shuffle-shard lookback period == query-ingesters-within |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find an answer to this: is the lookback period the same as the query-ingesters-within parameter or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is query-ingesters-within can we get confirmation from @pracucci ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shuffle sharding lookback period in Mimir is configured here:
https://github.com/grafana/mimir/blob/ab3e627b1da134f33c06dd09ee4402a2eaad68f1/pkg/mimir/modules.go#L448-L450
It's set equal to TSDB retention, but that's a recent change. In the past it was set to the query-ingesters-within parameter. The reason why we changed to TSDB retention is because now we let to customize the query-ingesters-within parameter on a per-tenant basis (via the live-reloaded runtime config) and so it's not the same setting for the entire cluster. The worst case scenario we want to cover is the TSDB retention period in ingesters (which is also set to 13h).
I think for your use case setting it to query-ingesters-within parameter is fine.
52ce46e
to
67b98b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
There's just a lint problem :)
What this PR does / why we need it:
Implements logic to enable queriers to query the right ingesters based on active partitions.
Which issue(s) this PR fixes:
Fixes https://github.com/grafana/loki-private/issues/1132
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
deprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR