Shuffle User Order in UsersScanner #10513
Conversation
|
|
dmwilson-grafana
changed the title
dmwilson-grafana
changed the title
There was a problem hiding this comment.
LGTM with some small suggestions. I'll let @seizethedave approve
| } | ||
| } | ||
|
|
||
| func TestUsersScanner_ScanUsers_ShouldReturnRandomizedOrder(t *testing.T) { |
There was a problem hiding this comment.
I'm not sure what this test is testing. It doesn't seem like it's ensuring a random order.
There was a problem hiding this comment.
Without the shuffle, the order of the input (users) would be the same as the order of actual. On L94, I check that we have the same set of users (as you mentioned, this can be changed to assert.ElementsMatch) and on L97 I check that it's actually been shuffled , i.e. users != actual.
A better test might involve setting a specific rand.Source and checking that it shuffles to a specific order.
CHANGELOG.md
Outdated
| @@ -35,6 +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 | |||
There was a problem hiding this comment.
Can this briefly say "why" in addition to "how"? So that unfamiliar readers understand why they might upgrade, and so on. :)
pkg/storage/tsdb/users_scanner.go
Outdated
| @@ -46,6 +47,10 @@ func (s *UsersScanner) ScanUsers(ctx context.Context) (users, markedForDeletion | |||
| return nil, nil, err | |||
| } | |||
|
|
|||
| rand.Shuffle(len(users), func(i, j int) { | |||
There was a problem hiding this comment.
I know we've changed course on this a time or two, but ScanUsers is a general purpose accessor function used by many services, whereas this shuffle behavior is particular to the needs of compactor. I actually think we can move the shuffle to runCleanup after we call refreshOwnedUsers, with a brief handrail comment noting why we'd do such a thing.
seizethedave
deleted the
dwilson/run-bucket-index-cleanup-by-index-age
branch
If there is a rollout, or any kind of graceful shutdown of compactors, they terminate ongoing block cleanup and bucket index update jobs. This can lead to the bucket index not being updated for an extended period if jobs are terminated back-to-back.
What this PR does
This PR addresses the above by shuffling the order of users submitted to
BlocksCleanerbyUsersScanner. When users are always processed in lexicographic order, successive restarts can cause read path failures for tenants with stale indexes. By randomizing this order, we reduce the probability that these indexes become stale.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.