Add partitions ring support to Distributor.UserStats() #7402
Conversation
| @@ -2996,28 +3332,25 @@ func TestDistributor_LabelValuesCardinality(t *testing.T) { | |||
| } | |||
|
|
|||
| tests := map[string]struct { | |||
| labelNames []model.LabelName | |||
| matchers []*labels.Matcher | |||
| ingestersSeriesCountTotal uint64 | |||
There was a problem hiding this comment.
Note to reviewers: I've modified this test to not mock the result of mockIngester.UserStats() anymore, but having it computing the actual number of pushed series.
| "partitions RF=2 (2 zones), 4 ingesters, all ingesters in the preferred zone (zone-a) are UNHEALTHY": { | ||
| ingesterStateByZone: map[string]ingesterZoneState{ | ||
| "zone-a": {numIngesters: 2, happyIngesters: 0}, |
There was a problem hiding this comment.
Is UNHEALTHY a ring term? In this case they're just returning 5xx, but are ACTIVE in the ring
There was a problem hiding this comment.
Is UNHEALTHY a ring term?
It's not a ring state, but it's what we display in the ring UI.
| numPartitions := 0 | ||
| for _, state := range testData.ingesterStateByZone { | ||
| numPartitions = max(numPartitions, max(state.numIngesters, len(state.states))) | ||
| } |
There was a problem hiding this comment.
you can maybe use prepConfig.totalIngesters() and prepConfig.totalZones() to figure this one out. It should work in this this test. If you still want to preserve the behaviour you implemented which supports different number of ingesters per zone, then, then can you add this as a method on prepConfig?
There was a problem hiding this comment.
Suggestion is to add a function on prepConfig to set a field on prepConfig itself. Don't think will make code better.
There was a problem hiding this comment.
the suggestion was to add maxIngestersPerZone() int function and use that here. Mainly because prepConfig already counts ingesters and zones and know what are the different ways of setting the number of ingesters (states vs happy/unhappy)
it's a minor change, i'll leave it up to you
There was a problem hiding this comment.
Instead of explicitly setting ingestStoragePartitions we can auto-detect it in most cases. I've done it here: 7916307
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
query-partitions-for-userstats
branch
from
237dd98
to
c217503
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…ats_ShouldSupportIngestStorage() because not necessary Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
query-partitions-for-userstats
branch
from
57232cf
to
1b44a0e
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
query-partitions-for-userstats
branch
from
1b44a0e
to
7916307
Compare
Addressed (see comment). |
What this PR does
In this PR I'm adding partitions ring support to
Distributor.UserStats(). Contrary to #7393 (which was straightforward) the main difference here is that with the ingest storage we have to account for replication in a slightly different way (see comments in the code).Note to reviewers:
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.