From b99a6cd9e1202b9e5cd5cadeea2b6c0c48c7ddf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Tue, 20 Aug 2024 11:15:41 +0200 Subject: [PATCH] Use pointers for readOnlyInstances and oldestReadOnlyUpdatedTimestamp. --- ring/ring.go | 32 ++++++++++++++------------------ ring/ring_test.go | 20 ++++++++++++-------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/ring/ring.go b/ring/ring.go index 2a8137d25..4aaaaa099 100644 --- a/ring/ring.go +++ b/ring/ring.go @@ -191,12 +191,11 @@ type Ring struct { // then this value will be 0. oldestRegisteredTimestamp int64 - readOnlyInstancesUpdated bool // True, if readOnlyInstances was actually updated. False if 0 is because of default value. - readOnlyInstances int // Number of instances with ReadOnly flag set. Only valid if readOnlyInstancesUpdated is set. - // Oldest value of ReadOnlyUpdatedTimestamp for read-only instances. If any read-only instance - // has ReadOnlyUpdatedTimestamp == 0 (which should not happen), then this value will be 0. - // Only valid if readOnlyInstancesUpdated is set. - oldestReadOnlyUpdatedTimestamp int64 + readOnlyInstances *int // Number of instances with ReadOnly flag set. Only valid if not nil. + // Oldest value of ReadOnlyUpdatedTimestamp for read-only instances. If there are no read-only instances, + // or if any read-only instance has ReadOnlyUpdatedTimestamp == 0 (which should not happen), then this value will be 0. + // Only valid if not nil. + oldestReadOnlyUpdatedTimestamp *int64 // Maps a token with the information of the instance holding it. This map is immutable and // cannot be changed in place because it's shared "as is" between subrings (the only way to @@ -395,9 +394,8 @@ func (r *Ring) updateRingState(ringDesc *Desc) { r.writableInstancesWithTokensCountPerZone = writableInstancesWithTokensCountPerZone r.oldestRegisteredTimestamp = oldestRegisteredTimestamp r.lastTopologyChange = now - r.readOnlyInstancesUpdated = true - r.readOnlyInstances = readOnlyInstances - r.oldestReadOnlyUpdatedTimestamp = oldestReadOnlyUpdatedTimestamp + r.readOnlyInstances = &readOnlyInstances + r.oldestReadOnlyUpdatedTimestamp = &oldestReadOnlyUpdatedTimestamp // Invalidate all cached subrings. if r.shuffledSubringCache != nil { @@ -884,16 +882,14 @@ func (r *Ring) filterOutReadOnlyInstances(lookbackPeriod time.Duration, now time r.mtx.RLock() defer r.mtx.RUnlock() - if r.readOnlyInstancesUpdated { - // If there are no read-only instances, there's no need to do any filtering. - if r.readOnlyInstances == 0 { - return r - } + // If there are no read-only instances, there's no need to do any filtering. + if r.readOnlyInstances != nil && *r.readOnlyInstances == 0 { + return r + } - // If all readOnlyUpdatedTimestamp values are within lookback window, we can return the ring without any filtering. - if lookbackPeriod > 0 && r.oldestReadOnlyUpdatedTimestamp >= lookbackUntil { - return r - } + // If all readOnlyUpdatedTimestamp values are within lookback window, we can return the ring without any filtering. + if lookbackPeriod > 0 && r.oldestReadOnlyUpdatedTimestamp != nil && *r.oldestReadOnlyUpdatedTimestamp >= lookbackUntil { + return r } shard := make(map[string]InstanceDesc, len(r.ringDesc.Ingesters)) diff --git a/ring/ring_test.go b/ring/ring_test.go index 8a2a785b3..5fb087873 100644 --- a/ring/ring_test.go +++ b/ring/ring_test.go @@ -2336,8 +2336,9 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) { ring.oldestRegisteredTimestamp = ringDesc.getOldestRegisteredTimestamp() } if updateReadOnlyInstances { - ring.readOnlyInstancesUpdated = true - ring.readOnlyInstances, ring.oldestReadOnlyUpdatedTimestamp = ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + readOnlyInstances, oldestReadOnlyUpdatedTimestamp := ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + ring.readOnlyInstances = &readOnlyInstances + ring.oldestReadOnlyUpdatedTimestamp = &oldestReadOnlyUpdatedTimestamp } case remove: delete(ringDesc.Ingesters, event.instanceID) @@ -2350,8 +2351,9 @@ func TestRing_ShuffleShardWithLookback(t *testing.T) { ring.oldestRegisteredTimestamp = ringDesc.getOldestRegisteredTimestamp() } if updateReadOnlyInstances { - ring.readOnlyInstancesUpdated = true - ring.readOnlyInstances, ring.oldestReadOnlyUpdatedTimestamp = ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + readOnlyInstances, oldestReadOnlyUpdatedTimestamp := ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + ring.readOnlyInstances = &readOnlyInstances + ring.oldestReadOnlyUpdatedTimestamp = &oldestReadOnlyUpdatedTimestamp } case test: rs, err := ring.ShuffleShardWithLookback(userID, event.shardSize, lookbackPeriod, now).GetAllHealthy(Read) @@ -2423,8 +2425,9 @@ func TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy(t *testing.T) { ring.oldestRegisteredTimestamp = ringDesc.getOldestRegisteredTimestamp() } if updateReadOnlyInstances { - ring.readOnlyInstancesUpdated = true - ring.readOnlyInstances, ring.oldestReadOnlyUpdatedTimestamp = ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + readOnlyInstances, oldestReadOnlyUpdatedTimestamp := ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + ring.readOnlyInstances = &readOnlyInstances + ring.oldestReadOnlyUpdatedTimestamp = &oldestReadOnlyUpdatedTimestamp } if len(ring.ringZones) != numZones { @@ -3310,8 +3313,9 @@ func benchmarkShuffleSharding(b *testing.B, numInstances, numZones, numTokens, s strategy: NewDefaultReplicationStrategy(), lastTopologyChange: time.Now(), } - ring.readOnlyInstances, ring.oldestReadOnlyUpdatedTimestamp = ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() - ring.readOnlyInstancesUpdated = true + readOnlyInstances, oldestReadOnlyUpdatedTimestamp := ringDesc.readOnlyInstancesAndOldestReadOnlyUpdatedTimestamp() + ring.readOnlyInstances = &readOnlyInstances + ring.oldestReadOnlyUpdatedTimestamp = &oldestReadOnlyUpdatedTimestamp b.ResetTimer()