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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache controllers #3589
Cache controllers #3589
Changes from all commits
bb9b23c
9d7898a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand the idea behind refreshing & clearing stale entries a little better.
IIUC when an entry becomes idle, we'd refresh it few times before deleting which seems counterintuitive.
If this is true, shouldn't we throttle refreshes only to entries which were read recently? Then Get should probably return false to force a refresh upon reading an idle (& not refreshed) entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose the simplest approach I thought would work (periodically refresh all entries, remove entries that weren't read in a while).
If
Get
returns false then the entry is effectively removed. So if I understand your idea correctly it's effectively the same as setting shorter lifetime for cache entries (less than 2 refresh durations?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's about simplicity then why not just:
This reduces number of calls to scaleNamespacer.Get as we never call it for entires that are no longer present.
I understand that the drawback of this solution is that all Gets are executed in the main VPA loop instead of being spread evenly over time.
So basically the discussion boils down to a question: "how long do we actively use an entry, on average"; if it's much longer than scaleCacheEntryLifetime, the amortised cost of multiple refreshes of a no-longer-used entry is low and then the gain from spreading scaleNamespacer.Get calls is likely more important.
If you think we'd indeed have long-lived entries (seems plausible) let's leave the solution as it is now.
I'd love to see some metric (or logs with counter) added at some point so we can assess the impact of this change and maybe fine-tune scaleCacheEntryLifetime vs scaleCacheEntryFreshnessTime, likely lowering the former.