Fix MaxUnavailableZones computed by PartitionInstanceRing.GetReplicationSetsForOperation()

[pracucci]

What this PR does:

PartitionInstanceRing.GetReplicationSetsForOperation() currently returns ReplicationSet with MaxUnavailableZones computed as len(instances) - 1. This works fine if all instances are in different zones, but we have some (or all) instances in the same zone it will not work as expected because MaxUnavailableZones will be >= the actual number of zones among instances. For example, when that's the case, the zoneAwareResultTracker will not issue any request at all because if MaxUnavailableZones is >= the actual number of zones... well the quorum is reached even without sending any request.

In this PR I propose to change MaxUnavailableZones to len(zones) - 1. Naturally this has an impact on quorum if some (or all instances) run in the same zone, but we expect not to be the case when using the partitions ring. However, if that will happen (e.g. local dev env, a demo, ...) after this change the system will keep working as expected, instead of weird bugs being surfaced (e.g. no requests sent to instances at all).

Benchmark:

goos: darwin
goarch: amd64
pkg: github.com/grafana/dskit/ring
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
                                                        │ before.txt  │             after.txt              │
                                                        │   sec/op    │   sec/op     vs base               │
PartitionInstanceRing_GetReplicationSetsForOperation-12   37.99µ ± 1%   41.91µ ± 1%  +10.33% (p=0.002 n=6)

                                                        │  before.txt  │            after.txt            │
                                                        │     B/op     │     B/op      vs base           │
PartitionInstanceRing_GetReplicationSetsForOperation-12   32.88Ki ± 0%   32.88Ki ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal

                                                        │ before.txt │           after.txt           │
                                                        │ allocs/op  │ allocs/op   vs base           │
PartitionInstanceRing_GetReplicationSetsForOperation-12   101.0 ± 0%   101.0 ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal

Which issue(s) this PR fixes:

N/A

Checklist

• Tests updated
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[dimitarvdimitrov]

we have some (or all) instances in the same zone

With the implementation in this PR would we end up querying both instances in the same zone or only one of them (assuming we use DoUntilQuorum)?

[pracucci]

we have some (or all) instances in the same zone

With the implementation in this PR would we end up querying both instances in the same zone or only one of them (assuming we use DoUntilQuorum)?

Both. That's what I meant by 👇

Naturally this has an impact on quorum if some (or all instances) run in the same zone, but we expect not to be the case when using the partitions ring. However, if that will happen (e.g. local dev env, a demo, ...) after this change the system will keep working as expected, instead of weird bugs being surfaced (e.g. no requests sent to instances at all).

[dimitarvdimitrov]

LGTM, but TestDefaultResultTracker_StartMinimumRequests_NoFailingRequests is failing and it sounds somewhat related. Have you checked it?

[pracucci]

TestDefaultResultTracker_StartMinimumRequests_NoFailingRequests

I don't think it's related because the modified code doesn't touch the code run by that test at all, but I'm not in a hurry on this PR and tomorrow I will look at why TestDefaultResultTracker_StartMinimumRequests_NoFailingRequests is flaky.

[pracucci]

TestDefaultResultTracker_StartMinimumRequests_NoFailingRequests

I don't think it's related because the modified code doesn't touch the code run by that test at all, but I'm not in a hurry on this PR and tomorrow I will look at why TestDefaultResultTracker_StartMinimumRequests_NoFailingRequests is flaky.

Yes, it's unrelated: #491

[dimitarvdimitrov]

LGTM

[dimitarvdimitrov]

the reverse partition-zone naming is confusing

[pracucci]

Agree. Fixed in a2609c7