distributor: make unit tests more flexible #7205
Conversation
This PR is a prerequisite to work that pstibrany and I are doing to support a ring for partitions. The major changes in this PR are: * add the ability to specify ingester state by zone; this allows to have a different number of failing ingesters per zone; this is useful in testing the ring retry mechanisms * add the ability to specify ingester ring state by zone * add the ability to inject data into `mockIngester`s; this removes the need to push data to ingesters in order ot test query logic. This is necessary when testing ingest storage and removes the need to spin up a kafka-compatible backend and consume from it in order to assert on querying logic Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
| for i := cfg.happyIngesters; i < cfg.numIngesters; i++ { | ||
| ingesters = append(ingesters, mockIngester{ | ||
| queryDelay: cfg.queryDelay, | ||
| pushDelay: cfg.pushDelay, | ||
| seriesCountTotal: cfg.ingestersSeriesCountTotal, | ||
| }) | ||
| } |
There was a problem hiding this comment.
previously unhappy ingesters didn't have a zone set, I think this was a bug
| for i := range ingesters { | ||
| addr := fmt.Sprintf("%d", i) |
There was a problem hiding this comment.
the address was opaque, so changing it to something more involved shouldn't make a difference
This reverts commit 92cd090.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pkg/distributor/distributor_test.go
Outdated
| if len(metrics) == 0 { | ||
| metrics = []string{"foo"} | ||
| } |
There was a problem hiding this comment.
[nit] Have you considered always specifying it? I understand it may require many changes to callers, but would make the logic in the caller easier to follow because it's not based on the assupmption that "no metrics" mean "foo" metric.
There was a problem hiding this comment.
i don't mind. I think it will make the tests easier to follow. done in 69b2c0f
| return ingestersRing.InstancesCount() | ||
| }) | ||
|
|
||
| factory := ring_client.PoolInstFunc(func(inst ring.InstanceDesc) (ring_client.PoolClient, error) { | ||
| return ingestersByAddr[inst.Addr], nil | ||
| for i := range ingesters { | ||
| ing := &ingesters[i] // Take a pointer, so we don't copy the sync.Mutex in the mockIngester |
There was a problem hiding this comment.
[non blocking] In a follow up PR we may consider changing mockIngester to be a pointer everywhere.
There was a problem hiding this comment.
I opened #7223
|
|
||
| } | ||
|
|
||
| func prepareIngesterZone(zone string, state ingesterZoneState, cfg prepConfig) []mockIngester { |
There was a problem hiding this comment.
[nit] Plural ingesters?
| func prepareIngesterZone(zone string, state ingesterZoneState, cfg prepConfig) []mockIngester { | |
| func prepareIngestersZone(zone string, state ingesterZoneState, cfg prepConfig) []mockIngester { |
There was a problem hiding this comment.
it's one zone of ingesters. Here ingester is an adjective, not a noun.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
deleted the
dimitar/distributor/add-testing-fscilities-for-partitions-testing
branch
This PR is a prerequisite to the work that @pstibrany, @pracucci and I are doing to support a ring for partitions.
The major changes in this PR are:
mockIngesters; this removes the need to push data to ingesters in order ot test query logic. This is necessary when testing ingest storage and removes the need to spin up a kafka-compatible backend and consume from it in order to assert on querying logicThese are the kind of tests this PR enables