ring: add GetWithOptions method to adjust per call behavior #632
Conversation
| @@ -1332,36 +1420,3 @@ func (op Operation) ShouldExtendReplicaSetOnState(s InstanceState) bool { | |||
|
|
|||
| // All states are healthy, no states extend replica set. | |||
| var allStatesRingOperation = Operation(0x0000ffff) | |||
|
|
|||
| // numberOfKeysOwnedByInstance returns how many of the supplied keys are owned by given instance. | |||
| func (r *Ring) numberOfKeysOwnedByInstance(keys []uint32, op Operation, instanceID string, bufDescs []InstanceDesc, bufHosts []string, bufZones []string) (int, error) { | |||
There was a problem hiding this comment.
This was only called from tests. I've moved most of this logic to the test in question (token_range_test.go).
56quarters
force-pushed
the
56quarters/sg-replication
branch
2 times, most recently
from
35571f9 to
e17f2cc
Compare
| @@ -627,27 +628,31 @@ func TestRing_Get_ZoneAwareness(t *testing.T) { | |||
| "should succeed if there are enough instances per zone on RF = 3": { | |||
| numInstances: 16, | |||
| numZones: 3, | |||
| totalZones: 3, | |||
| replicationFactor: 3, | |||
| zoneAwarenessEnabled: true, | |||
| expectedInstances: 3, | |||
| }, | |||
| "should fail if there are instances in 1 zone only on RF = 3": { | |||
There was a problem hiding this comment.
Without the change to add an instance in each zone below, this test would fail.
Previously, findInstancesForKey ignored the actual number of zones, len(r.ringTokensByZone), and assumed that replication factor was equivalent. Now that we've changed things so that replication factor is not necessarily the same as the number of zones, we need to add at least one instance in each zone so that findInstancesForKey is aware of them and only picks a single instance from each zone.
| distinctHosts = bufHosts[:0] | ||
| distinctZones = bufZones[:0] | ||
|
|
||
| // TODO: Do we need to pass this in to avoid allocations? |
There was a problem hiding this comment.
I'd like to replace passing bufDescs, bufHosts, bufZones around everywhere with a single struct Buffers which could include this map but that was a pretty invasive and non-backwards compatible change (not that we have any guarantees in dskit).
This change adds a new method that accepts 0 or more `Option` instances that modify the behavior of the call. These options can (currently) be used to adjust the replication factor for a particular key or use buffers to avoid excessive allocation. The most notable changes are in the `Ring.findInstancesForKey` method which is the core of the `Ring.Get` method. Instead of keeping track of distinct zones and assuming that only a single instance per zone would ever be returned, we keep a map of the number of instances found in each zone. Part of grafana/mimir#9944
Signed-off-by: Nick Pillitteri <[email protected]>
56quarters
force-pushed
the
56quarters/sg-replication
branch
from
6c1dec6 to
7b887a7
Compare
What this PR does:
This change adds a new method that accepts 0 or more
Optioninstances that modify the behavior of the call. These options can (currently) be used to adjust the replication factor for a particular key or use buffers to avoid excessive allocation.The most notable changes are in the
Ring.findInstancesForKeymethod which is the core of theRing.Getmethod. Instead of keeping track of distinct zones and assuming that only a single instance per zone would ever be returned, we keep a map of the number of instances found in each zone.Which issue(s) this PR fixes:
Part of grafana/mimir#9944
Previous attempt #620
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]