use default retention period to check user index may have expired chunks when user does not have custom retention #5261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nks when user does not have custom retention
cyriltovena
reviewed
Jan 28, 2022
| tenantsRetention *TenantsRetention | ||
| latestRetentionStartTime model.Time | ||
| tenantsRetention *TenantsRetention | ||
| // overallLatestRetentionStartTime holds latest retention start time considering both default and per user retention config. |
There was a problem hiding this comment.
Suggested change
| // overallLatestRetentionStartTime holds latest retention start time considering both default and per user retention config. | |
| // overallLatestRetentionStartTime holds latest retention start time for all users considering both default and per user retention config. |
cyriltovena
reviewed
Jan 28, 2022
| // findLatestRetentionStartTime returns the latest retention start time overall and by each user. | ||
| func findLatestRetentionStartTime(now model.Time, limits Limits) (model.Time, map[string]model.Time) { | ||
| // findLatestRetentionStartTime returns the latest retention start time overall, just default config and by each user. | ||
| func findLatestRetentionStartTime(now model.Time, limits Limits) (model.Time, model.Time, map[string]model.Time) { |
There was a problem hiding this comment.
nit: I'm not happy with the return here anymore. I think in the future we should make that a struct with proper names, this would avoid confusion.
There was a problem hiding this comment.
makes sense, pushed this change
cyriltovena
reviewed
Jan 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
When checking whether user specific index tables may have any expired chunks, we were using the smallest overall retention period instead of the smallest default retention period.
This causes us to unnecessarily download user index tables in some cases, for e.g:
We have a default config of
30d,user1has a custom stream retention of24handuser2has no retention config.Here the overall smallest retention period is
24hand the smallest default retention is30d.When checking if
user2's index table may have expired chunks based on table interval, we should be considering30dretention and not24h.Checklist