Add partitions ring support to more Distributor querying functions #7393
Conversation
| @@ -1505,6 +1507,29 @@ func forReplicationSet[T any](ctx context.Context, d *Distributor, replicationSe | |||
| return ring.DoUntilQuorum(ctx, replicationSet, d.queryQuorumConfig(ctx, replicationSet), wrappedF, cleanup) | |||
| } | |||
|
|
|||
| // forReplicationSets runs f, in parallel, for all ingesters in the input replication sets. | |||
| func forReplicationSets[R any](ctx context.Context, d *Distributor, replicationSets []ring.ReplicationSet, f func(context.Context, ingester_client.IngesterClient) (R, error)) ([]R, error) { | |||
There was a problem hiding this comment.
Note to reviewers: this function contains some duplicate code from forReplicationSet(), but forReplicationSet() will disappear in few PRs.
| @@ -1553,10 +1554,6 @@ func TestDistributor_Push_HistogramValidation(t *testing.T) { | |||
| assert.Nil(t, resp) | |||
| checkGRPCError(t, tc.expectedErr, expectedDetails, err) | |||
| } | |||
|
|
|||
| t.Cleanup(func() { | |||
There was a problem hiding this comment.
Note to reviewers: this is already done by prepare(). I removed it both here and in few other tests.
| @@ -2698,58 +2749,78 @@ func TestDistributor_LabelNamesAndValues(t *testing.T) { | |||
| Values: []string{"200", "500"}, | |||
| }, | |||
| } | |||
| tests := map[string]struct { | |||
| zones []string | |||
| zonesResponseDelay map[string]time.Duration | |||
There was a problem hiding this comment.
Note to reviewers: the test case on zone awareness only makes sense when partitions are not used (quorum works differently with partitions). So I've extracted this test case in a dedicated t.Run().
pracucci
changed the title
Added partitions support to Distributor.LabelNamesAndValues() Added partitions support to Distributor.LabelValuesForLabelName() Added partitions support to Distributor.MetricsForLabelMatchers() Added partitions support to Distributor.MetricsMetadata() Added partitions support to Distributor.QueryExemplars() Signed-off-by: Marco Pracucci <marco@pracucci.com> Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pracucci
force-pushed
the
query-partitions-for-labels
branch
from
0ffbaac
to
27091a2
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
| testConfig.limits = prepareDefaultLimits() | ||
| testConfig.limits.IngestionPartitionsTenantShardSize = testData.shuffleShardSize |
There was a problem hiding this comment.
aren't these being done in prepare already?
you can simplify this if/else by a lot
There was a problem hiding this comment.
Here I'm setting IngestionPartitionsTenantShardSize, and not IngestionTenantShardSize. When I test the ingest storage I (intentionally) don't want to set IngestionTenantShardSize, and viceversa when testing with the ingest storage disabled, in order to make sure the right config option is used for shuffle sharding. For this reason, I would need the if condition even if I will move IngestionPartitionsTenantShardSize as a prepConfig (due to the lack of a ternary operator in go).
I'm going to keep it as is for now.
There was a problem hiding this comment.
yeah makes sense. I didn't read the full config option
Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
This PR, which builds on #7388, add partitions ring support to the following Distributor functions:
LabelNames()LabelNamesAndValues()LabelValuesForLabelName()Distributor.MetricsForLabelMatchers()Distributor.MetricsMetadata()Distributor.QueryExemplars()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.