-
Notifications
You must be signed in to change notification settings - Fork 512
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
Add partitions ring support to Distributor.QueryStream() #7388
Conversation
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.
leaving a partial review, will continue reviewing tomorrow
if len(replicationSets) == 1 { | ||
zoneSorter = queryIngestersRingZoneSorter(replicationSets[0]) | ||
} |
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.
nitpick, but i'd even panic if that's not the case
} | ||
} | ||
|
||
// LabelValuesForLabelName returns all of the label values that are associated with a given label name. | ||
func (d *Distributor) LabelValuesForLabelName(ctx context.Context, from, to model.Time, labelName model.LabelName, matchers ...*labels.Matcher) ([]string, error) { | ||
replicationSet, err := d.GetIngesters(ctx) | ||
//nolint:staticcheck |
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.
is this because of the deprecated notice?
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, exactly.
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 usage of deprecated functions will disappear as soon as I complete this work (over the course of few more PRs).
bb43026
to
1af25c8
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
1af25c8
to
3414ac9
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…estStorage Signed-off-by: Marco Pracucci <marco@pracucci.com>
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.
Very nice set of tests. Good job @dimitarvdimitrov and @pracucci!
expectedQueriedIngesters: 5 /* zone-a */ + 2, /* the two failed fall back to zone-b */ | ||
expectedErr: errFail, | ||
}, | ||
"should succeed if there unhealthy ingesters in every zone but unhealthy ingesters own different partitions": { |
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.
This is very nice property.
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 coolest property of the new system ;)
All done by Dimitar! |
I thought so, but wasn't sure :) |
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
Going to merge it since I don't think that what we do in this PR is particularly contentious. I there will be post merge comments, I will quickly address them in a follow up PR. |
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, left an idea for one more test case, but happy to do it in a follow-up PR if you don't want to wait for CI on this one again
"should fail if all the ingesters owning a partition are in JOINING state": { | ||
ingesterStateByZone: map[string]ingesterZoneState{ | ||
"zone-a": {numIngesters: 5, happyIngesters: 5, ringStates: []ring.InstanceState{ring.JOINING, ring.ACTIVE, ring.ACTIVE, ring.ACTIVE, ring.ACTIVE}}, | ||
"zone-b": {numIngesters: 5, happyIngesters: 5, ringStates: []ring.InstanceState{ring.JOINING, ring.ACTIVE, ring.ACTIVE, ring.ACTIVE, ring.ACTIVE}}, | ||
}, |
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.
you can also add a case where different ingesters in the two zones are LEAVING (or JOINING) but the query still succeeds
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.
Good idea, done in 19650e3
…estStorage Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
In this PR I'm adding partitions ring support to
Distributor.QueryStream()
. Other Distributor's query functions don't use partitions ring yet and their support will be introduced in follow up PRs.Notes:
-querier.prefer-availability-zone
. The config is hidden in the doc because currently used only by experimental ingest storage.Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.