From 36276dc55ba41a3e218f24f326b0be4c7dc0729a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Wed, 13 Dec 2023 10:48:36 +0100 Subject: [PATCH 1/3] Simplify spread_minimizing_token_generator.go a bit by storing instance prefix. --- ring/spread_minimizing_token_generator.go | 58 ++++++---------- .../spread_minimizing_token_generator_test.go | 68 ++++--------------- 2 files changed, 34 insertions(+), 92 deletions(-) diff --git a/ring/spread_minimizing_token_generator.go b/ring/spread_minimizing_token_generator.go index 236382507..ed2613131 100644 --- a/ring/spread_minimizing_token_generator.go +++ b/ring/spread_minimizing_token_generator.go @@ -10,8 +10,6 @@ import ( "github.com/go-kit/log" "github.com/go-kit/log/level" - "github.com/pkg/errors" - "golang.org/x/exp/slices" ) @@ -22,11 +20,10 @@ const ( ) var ( - instanceIDRegex = regexp.MustCompile(`^(.*)-(\d+)$`) + instanceIDRegex = regexp.MustCompile(`^(.*-)(\d+)$`) errorBadInstanceIDFormat = func(instanceID string) error { return fmt.Errorf("unable to extract instance id from %q", instanceID) } - errorNoPreviousInstance = fmt.Errorf("impossible to find the instance preceding the target instance, because it is the first instance") errorMissingPreviousInstance = func(requiredInstanceID string) error { return fmt.Errorf("the instance %q has not been registered to the ring or has no tokens yet", requiredInstanceID) @@ -50,7 +47,7 @@ var ( type SpreadMinimizingTokenGenerator struct { instanceID int - instance string + instancePrefix string zoneID int spreadMinimizingZones []string canJoinEnabled bool @@ -58,6 +55,15 @@ type SpreadMinimizingTokenGenerator struct { } func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZones []string, canJoinEnabled bool, logger log.Logger) (*SpreadMinimizingTokenGenerator, error) { + prefix, instanceID, err := parseInstanceID(instance) + if err != nil { + return nil, err + } + + return NewSpreadMinimizingTokenGeneratorForInstanceID(prefix, instanceID, zone, spreadMinimizingZones, canJoinEnabled, logger) +} + +func NewSpreadMinimizingTokenGeneratorForInstanceID(instancePrefix string, instanceID int, zone string, spreadMinimizingZones []string, canJoinEnabled bool, logger log.Logger) (*SpreadMinimizingTokenGenerator, error) { if len(spreadMinimizingZones) <= 0 || len(spreadMinimizingZones) > maxZonesCount { return nil, errorZoneCountOutOfBound(len(spreadMinimizingZones)) } @@ -66,10 +72,6 @@ func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZo if !slices.IsSorted(sortedZones) { sort.Strings(sortedZones) } - instanceID, err := parseInstanceID(instance) - if err != nil { - return nil, err - } zoneID, err := findZoneID(zone, sortedZones) if err != nil { return nil, err @@ -77,7 +79,7 @@ func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZo tokenGenerator := &SpreadMinimizingTokenGenerator{ instanceID: instanceID, - instance: instance, + instancePrefix: instancePrefix, zoneID: zoneID, spreadMinimizingZones: sortedZones, canJoinEnabled: canJoinEnabled, @@ -86,32 +88,13 @@ func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZo return tokenGenerator, nil } -func parseInstanceID(instanceID string) (int, error) { - parts := instanceIDRegex.FindStringSubmatch(instanceID) - if len(parts) != 3 { - return 0, errorBadInstanceIDFormat(instanceID) - } - return strconv.Atoi(parts[2]) -} - -// previousInstance determines the string id of the instance preceding the given instance string id. -// If it is impossible to parse the given instanceID, or it is impossible to determine its predecessor -// because the passed instanceID has a bad format, or has no predecessor, an error is returned. -// For examples, my-instance-1 is preceded by instance my-instance-0, but my-instance-0 has no -// predecessor because its index is 0. -func previousInstance(instanceID string) (string, error) { +func parseInstanceID(instanceID string) (string, int, error) { parts := instanceIDRegex.FindStringSubmatch(instanceID) if len(parts) != 3 { - return "", errorBadInstanceIDFormat(instanceID) - } - id, err := strconv.Atoi(parts[2]) - if err != nil { - return "", err + return "", 0, errorBadInstanceIDFormat(instanceID) } - if id == 0 { - return "", errorNoPreviousInstance - } - return fmt.Sprintf("%s-%d", parts[1], id-1), nil + val, err := strconv.Atoi(parts[2]) + return parts[1], val, err } // findZoneID gets a zone name and a slice of sorted zones, @@ -339,13 +322,10 @@ func (t *SpreadMinimizingTokenGenerator) CanJoin(instances map[string]InstanceDe return nil } - prevInstance, err := previousInstance(t.instance) - if err != nil { - if errors.Is(err, errorNoPreviousInstance) { - return nil - } - return err + if t.instanceID == 0 { + return nil } + prevInstance := fmt.Sprintf("%s%d", t.instancePrefix, t.instanceID-1) instanceDesc, ok := instances[prevInstance] if ok && len(instanceDesc.Tokens) != 0 { return nil diff --git a/ring/spread_minimizing_token_generator_test.go b/ring/spread_minimizing_token_generator_test.go index cd49da405..ada794d38 100644 --- a/ring/spread_minimizing_token_generator_test.go +++ b/ring/spread_minimizing_token_generator_test.go @@ -28,21 +28,25 @@ var ( func TestSpreadMinimizingTokenGenerator_ParseInstanceID(t *testing.T) { tests := map[string]struct { - instanceID string - expectedID int - expectedError error + instanceID string + expectedPrefix string + expectedID int + expectedError error }{ "instance-zone-a-10 is correct": { - instanceID: "instance-zone-a-10", - expectedID: 10, + instanceID: "instance-zone-a-10", + expectedPrefix: "instance-zone-a-", + expectedID: 10, }, "instance-zone-b-0 is correct": { - instanceID: "instance-zone-b-0", - expectedID: 0, + instanceID: "instance-zone-b-0", + expectedPrefix: "instance-zone-b-", + expectedID: 0, }, "store-gateway-zone-c-7 is correct": { - instanceID: "store-gateway-zone-c-7", - expectedID: 7, + instanceID: "store-gateway-zone-c-7", + expectedPrefix: "store-gateway-zone-c-", + expectedID: 7, }, "instance-zone-c is not valid": { instanceID: "instance-zone-c", @@ -54,60 +58,18 @@ func TestSpreadMinimizingTokenGenerator_ParseInstanceID(t *testing.T) { }, } for _, testData := range tests { - id, err := parseInstanceID(testData.instanceID) + prefix, id, err := parseInstanceID(testData.instanceID) if testData.expectedError != nil { require.Error(t, err) require.Equal(t, testData.expectedError, err) } else { require.NoError(t, err) + require.Equal(t, testData.expectedPrefix, prefix) require.Equal(t, testData.expectedID, id) } } } -func TestSpreadMinimizingTokenGenerator_PreviousInstanceID(t *testing.T) { - tests := map[string]struct { - instanceID string - expectedInstanceID string - expectedError error - }{ - "previous instance of instance-zone-a-10 is instance-zone-a-9": { - instanceID: "instance-zone-a-10", - expectedInstanceID: "instance-zone-a-9", - }, - "previous instance of instance-zone-b-1 is instance-zone-b-0": { - instanceID: "instance-zone-b-1", - expectedInstanceID: "instance-zone-b-0", - }, - "previous instance of store-gateway-zone-c-1000 is store-gateway-zone-c-999": { - instanceID: "store-gateway-zone-c-1000", - expectedInstanceID: "store-gateway-zone-c-999", - }, - "instance-zone-0 has no previous instance": { - instanceID: "instance-zone-0", - expectedError: errorNoPreviousInstance, - }, - "instance-zone-c is not valid": { - instanceID: "instance-zone-c", - expectedError: errorBadInstanceIDFormat("instance-zone-c"), - }, - "empty instance is not valid": { - instanceID: "", - expectedError: errorBadInstanceIDFormat(""), - }, - } - for _, testData := range tests { - id, err := previousInstance(testData.instanceID) - if testData.expectedError != nil { - require.Error(t, err) - require.Equal(t, testData.expectedError, err) - } else { - require.NoError(t, err) - require.Equal(t, testData.expectedInstanceID, id) - } - } -} - func TestSpreadMinimizingTokenGenerator_FindZoneID(t *testing.T) { tests := map[string]struct { zone string From 1747a246f1d36538dd85a917efd650f8702a95b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Wed, 13 Dec 2023 13:55:41 +0100 Subject: [PATCH 2/3] Remove unnecessary fields from SpreadMinimizingTokenGenerator --- ring/basic_lifecycler_test.go | 2 +- ring/lifecycler_test.go | 10 +-- ring/spread_minimizing_token_generator.go | 77 +++++++++---------- .../spread_minimizing_token_generator_test.go | 29 ++++--- 4 files changed, 60 insertions(+), 58 deletions(-) diff --git a/ring/basic_lifecycler_test.go b/ring/basic_lifecycler_test.go index 08708f5be..3eb671e4d 100644 --- a/ring/basic_lifecycler_test.go +++ b/ring/basic_lifecycler_test.go @@ -25,7 +25,7 @@ const ( func TestBasicLifecycler_GetTokenGenerator(t *testing.T) { cfg := prepareBasicLifecyclerConfig() - spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(cfg.ID, cfg.Zone, []string{zone(1), zone(2), zone(3)}, true, log.NewNopLogger()) + spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(cfg.ID, cfg.Zone, []string{zone(1), zone(2), zone(3)}, true) require.NoError(t, err) tests := []TokenGenerator{nil, NewRandomTokenGenerator(), spreadMinimizingTokenGenerator} diff --git a/ring/lifecycler_test.go b/ring/lifecycler_test.go index 44b894ae9..e4ce79911 100644 --- a/ring/lifecycler_test.go +++ b/ring/lifecycler_test.go @@ -70,7 +70,7 @@ func TestLifecyclerConfig_Validate(t *testing.T) { require.NoError(t, err) require.Equal(t, pathToTokens, cfg.TokensFilePath) - spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(cfg.ID, cfg.Zone, []string{zone(1), zone(2), zone(3)}, true, log.NewNopLogger()) + spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(cfg.ID, cfg.Zone, []string{zone(1), zone(2), zone(3)}, true) require.NoError(t, err) cfg.RingTokenGenerator = spreadMinimizingTokenGenerator @@ -88,7 +88,7 @@ func TestLifecycler_TokenGenerator(t *testing.T) { cfg := testLifecyclerConfig(ringConfig, "instance-1") - spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(cfg.ID, cfg.Zone, []string{zone(1), zone(2), zone(3)}, true, log.NewNopLogger()) + spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(cfg.ID, cfg.Zone, []string{zone(1), zone(2), zone(3)}, true) require.NoError(t, err) tests := []TokenGenerator{nil, initTokenGenerator(t), spreadMinimizingTokenGenerator} @@ -1343,7 +1343,7 @@ func TestWaitBeforeJoining(t *testing.T) { spreadMinimizingZones := []string{zone(1), zone(2), zone(3)} canJoinTimeout := 5 * time.Second spreadMinimizingTokenGenerator := func(targetInstanceID string, canJoinEnabled bool) TokenGenerator { - tokenGenerator, err := NewSpreadMinimizingTokenGenerator(targetInstanceID, targetZone, spreadMinimizingZones, canJoinEnabled, log.NewNopLogger()) + tokenGenerator, err := NewSpreadMinimizingTokenGenerator(targetInstanceID, targetZone, spreadMinimizingZones, canJoinEnabled) require.NoError(t, err) return tokenGenerator } @@ -1476,7 +1476,7 @@ func TestAutoJoinWithSpreadMinimizingTokenGenerator(t *testing.T) { // Token generator of the instance with id 0 will call CanJoin with a delay of canJoinDelay, // in such a way that instance with id 1 waits for it. - tokenGeneratorWithDelay, err := newSpreadMinimizingTokenGeneratorWithDelay(firstInstanceID, zoneID, spreadMinimizingZones, true, canJoinDelay, log.NewNopLogger()) + tokenGeneratorWithDelay, err := newSpreadMinimizingTokenGeneratorWithDelay(firstInstanceID, zoneID, spreadMinimizingZones, true, canJoinDelay) require.NoError(t, err) firstCfg := testLifecyclerConfig(ringConfig, firstInstanceID) firstCfg.NumTokens = optimalTokensPerInstance @@ -1486,7 +1486,7 @@ func TestAutoJoinWithSpreadMinimizingTokenGenerator(t *testing.T) { require.NoError(t, services.StartAndAwaitRunning(context.Background(), firstLifecycler)) defer services.StopAndAwaitTerminated(context.Background(), firstLifecycler) //nolint:errcheck - tokenGenerator, err := NewSpreadMinimizingTokenGenerator(secondInstanceID, zoneID, spreadMinimizingZones, true, log.NewNopLogger()) + tokenGenerator, err := NewSpreadMinimizingTokenGenerator(secondInstanceID, zoneID, spreadMinimizingZones, true) require.NoError(t, err) secondCfg := testLifecyclerConfig(ringConfig, secondInstanceID) secondCfg.NumTokens = optimalTokensPerInstance diff --git a/ring/spread_minimizing_token_generator.go b/ring/spread_minimizing_token_generator.go index ed2613131..9adad7d39 100644 --- a/ring/spread_minimizing_token_generator.go +++ b/ring/spread_minimizing_token_generator.go @@ -8,8 +8,6 @@ import ( "sort" "strconv" - "github.com/go-kit/log" - "github.com/go-kit/log/level" "golang.org/x/exp/slices" ) @@ -46,24 +44,13 @@ var ( ) type SpreadMinimizingTokenGenerator struct { - instanceID int - instancePrefix string - zoneID int - spreadMinimizingZones []string - canJoinEnabled bool - logger log.Logger + instanceID int + instancePrefix string + zoneID int + canJoinEnabled bool } -func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZones []string, canJoinEnabled bool, logger log.Logger) (*SpreadMinimizingTokenGenerator, error) { - prefix, instanceID, err := parseInstanceID(instance) - if err != nil { - return nil, err - } - - return NewSpreadMinimizingTokenGeneratorForInstanceID(prefix, instanceID, zone, spreadMinimizingZones, canJoinEnabled, logger) -} - -func NewSpreadMinimizingTokenGeneratorForInstanceID(instancePrefix string, instanceID int, zone string, spreadMinimizingZones []string, canJoinEnabled bool, logger log.Logger) (*SpreadMinimizingTokenGenerator, error) { +func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZones []string, canJoinEnabled bool) (*SpreadMinimizingTokenGenerator, error) { if len(spreadMinimizingZones) <= 0 || len(spreadMinimizingZones) > maxZonesCount { return nil, errorZoneCountOutOfBound(len(spreadMinimizingZones)) } @@ -77,13 +64,20 @@ func NewSpreadMinimizingTokenGeneratorForInstanceID(instancePrefix string, insta return nil, err } + prefix, instanceID, err := parseInstanceID(instance) + if err != nil { + return nil, err + } + + return NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID(prefix, instanceID, zoneID, canJoinEnabled) +} + +func NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID(instancePrefix string, instanceID, zoneID int, canJoinEnabled bool) (*SpreadMinimizingTokenGenerator, error) { tokenGenerator := &SpreadMinimizingTokenGenerator{ - instanceID: instanceID, - instancePrefix: instancePrefix, - zoneID: zoneID, - spreadMinimizingZones: sortedZones, - canJoinEnabled: canJoinEnabled, - logger: logger, + instanceID: instanceID, + instancePrefix: instancePrefix, + zoneID: zoneID, + canJoinEnabled: canJoinEnabled, } return tokenGenerator, nil } @@ -176,7 +170,11 @@ func (t *SpreadMinimizingTokenGenerator) GenerateTokens(requestedTokensCount int used[v] = true } - allTokens := t.generateAllTokens() + allTokens, err := t.generateAllTokens() + if err != nil { + // we were unable to generate required tokens, so we panic. + panic(err) + } uniqueTokens := make(Tokens, 0, requestedTokensCount) // allTokens is a sorted slice of tokens for instance t.cfg.InstanceID in zone t.cfg.zone @@ -197,11 +195,14 @@ func (t *SpreadMinimizingTokenGenerator) GenerateTokens(requestedTokensCount int // placed in the ring that already contains instances with all the ids lower that t.instanceID // is optimal. // Calls to this method will always return the same set of tokens. -func (t *SpreadMinimizingTokenGenerator) generateAllTokens() Tokens { - tokensByInstanceID := t.generateTokensByInstanceID() +func (t *SpreadMinimizingTokenGenerator) generateAllTokens() (Tokens, error) { + tokensByInstanceID, err := t.generateTokensByInstanceID() + if err != nil { + return nil, err + } allTokens := tokensByInstanceID[t.instanceID] slices.Sort(allTokens) - return allTokens + return allTokens, nil } // generateTokensByInstanceID generates the optimal number of tokens (optimalTokenPerInstance), @@ -209,13 +210,13 @@ func (t *SpreadMinimizingTokenGenerator) generateAllTokens() Tokens { // (with id t.instanceID). Generated tokens are not sorted, but they are distributed in such a // way that registered ownership of all the instances is optimal. // Calls to this method will always return the same set of tokens. -func (t *SpreadMinimizingTokenGenerator) generateTokensByInstanceID() map[int]Tokens { +func (t *SpreadMinimizingTokenGenerator) generateTokensByInstanceID() (map[int]Tokens, error) { firstInstanceTokens := t.generateFirstInstanceTokens() tokensByInstanceID := make(map[int]Tokens, t.instanceID+1) tokensByInstanceID[0] = firstInstanceTokens if t.instanceID == 0 { - return tokensByInstanceID + return tokensByInstanceID, nil } // tokensQueues is a slice of priority queues. Slice indexes correspond @@ -255,10 +256,8 @@ func (t *SpreadMinimizingTokenGenerator) generateTokensByInstanceID() map[int]To optimalTokenOwnership := t.optimalTokenOwnership(optimalInstanceOwnership, currInstanceOwnership, uint32(optimalTokensPerInstance-addedTokens)) highestOwnershipInstance := instanceQueue.Peek() if highestOwnershipInstance == nil || highestOwnershipInstance.ownership <= float64(optimalTokenOwnership) { - level.Warn(t.logger).Log("msg", "it was impossible to add a token because the instance with the highest ownership cannot satisfy the request", "added tokens", addedTokens+1, "highest ownership", highestOwnershipInstance.ownership, "requested ownership", optimalTokenOwnership) - // if this happens, it means that we cannot accommodate other tokens, so we panic - err := fmt.Errorf("it was impossible to add %dth token for instance with id %d in zone %s because the instance with the highest ownership cannot satisfy the requested ownership %d", addedTokens+1, i, t.spreadMinimizingZones[t.zoneID], optimalTokenOwnership) - panic(err) + // if this happens, it means that we cannot accommodate other tokens + return nil, fmt.Errorf("it was impossible to add %dth token for instance with id %d in zone id %d because the instance with the highest ownership cannot satisfy the requested ownership %d", addedTokens+1, i, t.zoneID, optimalTokenOwnership) } tokensQueue := tokensQueues[highestOwnershipInstance.item.instanceID] highestOwnershipToken := tokensQueue.Peek() @@ -271,10 +270,8 @@ func (t *SpreadMinimizingTokenGenerator) generateTokensByInstanceID() map[int]To token := highestOwnershipToken.item newToken, err := t.calculateNewToken(token, optimalTokenOwnership) if err != nil { - level.Error(t.logger).Log("msg", "it was impossible to calculate a new token because an error occurred", "err", err) - // if this happens, it means that we cannot accommodate additional tokens, so we panic - err := fmt.Errorf("it was impossible to calculate the %dth token for instance with id %d in zone %s", addedTokens+1, i, t.spreadMinimizingZones[t.zoneID]) - panic(err) + // if this happens, it means that we cannot accommodate additional tokens + return nil, fmt.Errorf("it was impossible to calculate the %dth token for instance with id %d in zone id %d", addedTokens+1, i, t.zoneID) } tokens = append(tokens, newToken) // add the new token to currInstanceTokenQueue @@ -300,7 +297,7 @@ func (t *SpreadMinimizingTokenGenerator) generateTokensByInstanceID() map[int]To tokensByInstanceID[i] = tokens // if this is the last iteration we return, so we avoid to call additional heap.Pushs if i == t.instanceID { - return tokensByInstanceID + return tokensByInstanceID, nil } // If there were some ignored instances, we put them back on the queue. @@ -314,7 +311,7 @@ func (t *SpreadMinimizingTokenGenerator) generateTokensByInstanceID() map[int]To heap.Push(&instanceQueue, newRingInstanceOwnershipInfo(i, currInstanceOwnership)) } - return tokensByInstanceID + return tokensByInstanceID, nil } func (t *SpreadMinimizingTokenGenerator) CanJoin(instances map[string]InstanceDesc) error { diff --git a/ring/spread_minimizing_token_generator_test.go b/ring/spread_minimizing_token_generator_test.go index ada794d38..ecebc6635 100644 --- a/ring/spread_minimizing_token_generator_test.go +++ b/ring/spread_minimizing_token_generator_test.go @@ -4,11 +4,9 @@ import ( "fmt" "math" "math/rand" - "os" "testing" "time" - "github.com/go-kit/log" "github.com/stretchr/testify/require" "golang.org/x/exp/slices" ) @@ -138,7 +136,7 @@ func TestSpreadMinimizingTokenGenerator_NewSpreadMinimizingTokenGenerator(t *tes for _, testData := range tests { instance := fmt.Sprintf("instance-%s-1", testData.zone) - tokenGenerator, err := NewSpreadMinimizingTokenGenerator(instance, testData.zone, testData.spreadMinimizingZones, true, log.NewNopLogger()) + tokenGenerator, err := NewSpreadMinimizingTokenGenerator(instance, testData.zone, testData.spreadMinimizingZones, true) if testData.expectedError != nil { require.Error(t, err) require.Equal(t, testData.expectedError, err) @@ -293,9 +291,11 @@ func TestSpreadMinimizingTokenGenerator_GenerateAllTokensIdempotent(t *testing.T for _, zone := range zones { instance := fmt.Sprintf("instance-%s-%d", zone, instanceID) tokenGenerator := createSpreadMinimizingTokenGenerator(t, instance, zone, zones) - tokens1 := tokenGenerator.generateAllTokens() + tokens1, err := tokenGenerator.generateAllTokens() + require.NoError(t, err) require.Len(t, tokens1, tokensPerInstance) - tokens2 := tokenGenerator.generateAllTokens() + tokens2, err := tokenGenerator.generateAllTokens() + require.NoError(t, err) require.True(t, tokens1.Equals(tokens2)) } } @@ -344,7 +344,9 @@ func TestSpreadMinimizingTokenGenerator_CheckTokenUniqueness(t *testing.T) { for _, zone := range zones { instance := fmt.Sprintf("instance-%s-%d", zone, instanceID) tokenGenerator := createSpreadMinimizingTokenGenerator(t, instance, zone, zones) - tokens := tokenGenerator.generateTokensByInstanceID() + tokens, err := tokenGenerator.generateTokensByInstanceID() + require.NoError(t, err) + for i := 0; i <= instanceID; i++ { tks := tokens[i] for _, token := range tks { @@ -373,7 +375,8 @@ func TestSpreadMinimizingTokenGenerator_GenerateTokens(t *testing.T) { instance := fmt.Sprintf("instance-%s-%d", zone, instanceID) tokenGenerator := createSpreadMinimizingTokenGenerator(t, instance, zone, zones) // this is the set of all sorted tokens assigned to instance - allTokens := tokenGenerator.generateAllTokens() + allTokens, err := tokenGenerator.generateAllTokens() + require.NoError(t, err) require.Len(t, allTokens, tokensPerInstance) takenTokens := make(Tokens, 0, tokensPerInstance) @@ -463,7 +466,8 @@ func TestSpreadMinimizingTokenGenerator_CanJoin(t *testing.T) { targetInstance := fmt.Sprintf("instance-%s-%d", zone, instanceID) tokenGenerator = createSpreadMinimizingTokenGenerator(t, targetInstance, zone, zones) // this is the set of all sorted tokens assigned to instance - allTokens := tokenGenerator.generateTokensByInstanceID() + allTokens, err := tokenGenerator.generateTokensByInstanceID() + require.NoError(t, err) require.Len(t, allTokens, instanceID+1) ringDesc := &Desc{} @@ -516,7 +520,8 @@ func createTokensForAllInstancesAndZones(t *testing.T, maxInstanceID, tokensPerI for _, zone := range zones { instance := fmt.Sprintf("instance-%s-%d", zone, maxInstanceID) tokenGenerator := createSpreadMinimizingTokenGenerator(t, instance, zone, zones) - tokensByInstance := tokenGenerator.generateTokensByInstanceID() + tokensByInstance, err := tokenGenerator.generateTokensByInstanceID() + require.NoError(t, err) for id, tokens := range tokensByInstance { if !slices.IsSorted(tokens) { slices.Sort(tokens) @@ -546,7 +551,7 @@ func createTokensForAllInstancesAndZones(t *testing.T, maxInstanceID, tokensPerI } func createSpreadMinimizingTokenGenerator(t testing.TB, instance, zone string, zones []string) *SpreadMinimizingTokenGenerator { - tokenGenerator, err := NewSpreadMinimizingTokenGenerator(instance, zone, zones, true, log.NewLogfmtLogger(os.Stdout)) + tokenGenerator, err := NewSpreadMinimizingTokenGenerator(instance, zone, zones, true) require.NoError(t, err) require.NotNil(t, tokenGenerator) return tokenGenerator @@ -575,8 +580,8 @@ type spreadMinimizingTokenGeneratorWithDelay struct { canJoinDelay time.Duration } -func newSpreadMinimizingTokenGeneratorWithDelay(instance, zone string, spreadMinimizingZones []string, canJoinEnabled bool, canJoinDelay time.Duration, logger log.Logger) (*spreadMinimizingTokenGeneratorWithDelay, error) { - spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(instance, zone, spreadMinimizingZones, canJoinEnabled, logger) +func newSpreadMinimizingTokenGeneratorWithDelay(instance, zone string, spreadMinimizingZones []string, canJoinEnabled bool, canJoinDelay time.Duration) (*spreadMinimizingTokenGeneratorWithDelay, error) { + spreadMinimizingTokenGenerator, err := NewSpreadMinimizingTokenGenerator(instance, zone, spreadMinimizingZones, canJoinEnabled) if err != nil { return nil, err } From ff0417e6ade0cde44e9e16fbc3b4d717397118dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Wed, 13 Dec 2023 13:59:46 +0100 Subject: [PATCH 3/3] NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID can't return error. --- ring/spread_minimizing_token_generator.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ring/spread_minimizing_token_generator.go b/ring/spread_minimizing_token_generator.go index 9adad7d39..bd2ed9970 100644 --- a/ring/spread_minimizing_token_generator.go +++ b/ring/spread_minimizing_token_generator.go @@ -69,17 +69,16 @@ func NewSpreadMinimizingTokenGenerator(instance, zone string, spreadMinimizingZo return nil, err } - return NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID(prefix, instanceID, zoneID, canJoinEnabled) + return NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID(prefix, instanceID, zoneID, canJoinEnabled), nil } -func NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID(instancePrefix string, instanceID, zoneID int, canJoinEnabled bool) (*SpreadMinimizingTokenGenerator, error) { - tokenGenerator := &SpreadMinimizingTokenGenerator{ +func NewSpreadMinimizingTokenGeneratorForInstanceAndZoneID(instancePrefix string, instanceID, zoneID int, canJoinEnabled bool) *SpreadMinimizingTokenGenerator { + return &SpreadMinimizingTokenGenerator{ instanceID: instanceID, instancePrefix: instancePrefix, zoneID: zoneID, canJoinEnabled: canJoinEnabled, } - return tokenGenerator, nil } func parseInstanceID(instanceID string) (string, int, error) {