From f9cab59cb42874af64966a52bc7125415906b91a Mon Sep 17 00:00:00 2001 From: Dustin Wilson Date: Fri, 24 Jan 2025 17:27:51 -0500 Subject: [PATCH] move shuffle to refreshOwnedUsers --- CHANGELOG.md | 2 +- pkg/compactor/blocks_cleaner.go | 5 ++++ pkg/storage/tsdb/users_scanner.go | 5 ---- pkg/storage/tsdb/users_scanner_test.go | 33 ++++---------------------- 4 files changed, 10 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2af123cc62..b5841fbcd7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,7 +35,7 @@ * `cortex_ingester_tsdb_block_postings_for_matchers_cache_misses_total` * `cortex_ingester_tsdb_block_postings_for_matchers_cache_requests_total` * `cortex_ingester_tsdb_block_postings_for_matchers_cache_skips_total` -* [ENHANCEMENT] Compactor: Shuffle order of users in `UsersScanner` before submitting to `BlocksCleaner`. #10513 +* [ENHANCEMENT] Compactor: Shuffle users' order in `BlocksCleaner`. Prevents bucket indexes from going an extended period without cleanup during compactor restarts. #10513 * [BUGFIX] Distributor: Use a boolean to track changes while merging the ReplicaDesc components, rather than comparing the objects directly. #10185 * [BUGFIX] Querier: fix timeout responding to query-frontend when response size is very close to `-querier.frontend-client.grpc-max-send-msg-size`. #10154 * [BUGFIX] Query-frontend and querier: show warning/info annotations in some cases where they were missing (if a lazy querier was used). #10277 diff --git a/pkg/compactor/blocks_cleaner.go b/pkg/compactor/blocks_cleaner.go index 55bfeffcdf1..aac5b13d999 100644 --- a/pkg/compactor/blocks_cleaner.go +++ b/pkg/compactor/blocks_cleaner.go @@ -8,6 +8,7 @@ package compactor import ( "context" "fmt" + "math/rand" "strconv" "strings" "sync" @@ -232,6 +233,10 @@ func (c *BlocksCleaner) refreshOwnedUsers(ctx context.Context) ([]string, map[st return nil, nil, errors.Wrap(err, "failed to discover users from bucket") } + rand.Shuffle(len(users), func(i, j int) { + users[i], users[j] = users[j], users[i] + }) + isActive := util.StringsMap(users) isDeleted := util.StringsMap(deleted) allUsers := append(users, deleted...) diff --git a/pkg/storage/tsdb/users_scanner.go b/pkg/storage/tsdb/users_scanner.go index 1b7ca534c8f..0ade0f7a106 100644 --- a/pkg/storage/tsdb/users_scanner.go +++ b/pkg/storage/tsdb/users_scanner.go @@ -7,7 +7,6 @@ package tsdb import ( "context" - "math/rand" "strings" "github.com/go-kit/log" @@ -47,10 +46,6 @@ func (s *UsersScanner) ScanUsers(ctx context.Context) (users, markedForDeletion return nil, nil, err } - rand.Shuffle(len(users), func(i, j int) { - users[i], users[j] = users[j], users[i] - }) - // Check users for being owned by instance, and split users into non-deleted and deleted. // We do these checks after listing all users, to improve cacheability of Iter (result is only cached at the end of Iter call). for ix := 0; ix < len(users); { diff --git a/pkg/storage/tsdb/users_scanner_test.go b/pkg/storage/tsdb/users_scanner_test.go index 05d278b78b5..0af741d7778 100644 --- a/pkg/storage/tsdb/users_scanner_test.go +++ b/pkg/storage/tsdb/users_scanner_test.go @@ -8,7 +8,6 @@ package tsdb import ( "context" "errors" - "fmt" "path" "testing" @@ -37,10 +36,10 @@ func TestUsersScanner_ScanUsers_ShouldReturnedOwnedUsersOnly(t *testing.T) { } func TestUsersScanner_ScanUsers_ShouldReturnUsersForWhichOwnerCheckOrTenantDeletionCheckFailed(t *testing.T) { - users := []string{"user-1", "user-2"} + expected := []string{"user-1", "user-2"} bucketClient := &bucket.ClientMock{} - bucketClient.MockIter("", users, nil) + bucketClient.MockIter("", expected, nil) bucketClient.MockExists(path.Join("user-1", TenantDeletionMarkPath), false, nil) bucketClient.MockExists(path.Join("user-2", TenantDeletionMarkPath), false, errors.New("fail")) @@ -51,12 +50,11 @@ func TestUsersScanner_ScanUsers_ShouldReturnUsersForWhichOwnerCheckOrTenantDelet s := NewUsersScanner(bucketClient, isOwned, log.NewNopLogger()) actual, deleted, err := s.ScanUsers(context.Background()) require.NoError(t, err) - assert.ElementsMatch(t, actual, users) + assert.Equal(t, expected, actual) assert.Empty(t, deleted) } func TestUsersScanner_ScanUsers_ShouldNotReturnPrefixedUsedByMimirInternals(t *testing.T) { - users := []string{"user-1", "user-2"} bucketClient := &bucket.ClientMock{} bucketClient.MockIter("", []string{"user-1", "user-2", bucket.MimirInternalsPrefix}, nil) bucketClient.MockExists(path.Join("user-1", TenantDeletionMarkPath), false, nil) @@ -65,28 +63,5 @@ func TestUsersScanner_ScanUsers_ShouldNotReturnPrefixedUsedByMimirInternals(t *t s := NewUsersScanner(bucketClient, AllUsers, log.NewNopLogger()) actual, _, err := s.ScanUsers(context.Background()) require.NoError(t, err) - assert.ElementsMatch(t, actual, users) -} - -func TestUsersScanner_ScanUsers_ShouldReturnRandomizedOrder(t *testing.T) { - var users = make([]string, 20) - for i := 0; i < len(users); i++ { - users[i] = fmt.Sprintf("user-%d", i) - } - bucketClient := &bucket.ClientMock{} - bucketClient.MockIter("", users, nil) - for i := 0; i < len(users); i++ { - bucketClient.MockExists(path.Join(users[i], TenantDeletionMarkPath), false, nil) - } - - isOwned := func(_ string) (bool, error) { - return true, nil - } - - s := NewUsersScanner(bucketClient, isOwned, log.NewNopLogger()) - actual, deleted, err := s.ScanUsers(context.Background()) - require.NoError(t, err) - assert.ElementsMatch(t, actual, users) - assert.Empty(t, deleted) - assert.NotEqual(t, actual, users) + assert.Equal(t, []string{"user-1", "user-2"}, actual) }