-
Notifications
You must be signed in to change notification settings - Fork 544
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
Refactor caching to support multiple independent caches #3166
Conversation
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott We'll probably want to update the Upgrade page with information about this breaking change. |
Signed-off-by: Joe Elliott <[email protected]>
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 like the changes. Only have a couple of comments.
cache_min_compaction_level and cache_max_block_age were the only caching settings that were left alone. These are policy settings and I decided to leave them under storage.trace in the config block. Should we leave "policy" style settings in the normal config blocks? and only put caching settings in the cache provider? This is the approach I preferred but I thought it was worth discussing.
I like it the way it is right now.
Roles are currently configured as a yaml list, but I'm tempted to make them a pipe delimited list of roles. I think it would read better, but seems strange when yaml supports lists directly.
I actually it reads better as a list hah. I wouldn't change it.
Trace ID index had support for caching so I replicated it in this PR. Since this feature is known to be broken, I'm wondering if I should drop support until we invest the time to improve it.
Yeah, I think we should do that. Or at least log something when that cache is enabled. It's not very well documented that that cache is not working.
Co-authored-by: Mario <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
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.
LGTM
What this PR does:
This PR refactors caches to allow for multiple independent caches that can be assigned to various roles. It lays the ground work for adding additional caches for things like parquet pages or frontend search jobs. This PR is completely backwards compatible, but does deprecate all the existing cache configuration fields in the
store:
config block.writerCallback func(*backend.BlockMeta, time.Time)
from compactioncreateLegacyCache()
method.Open Questions
cache_min_compaction_level
andcache_max_block_age
were the only caching settings that were left alone. These are policy settings and I decided to leave them understorage.trace
in the config block. Should we leave "policy" style settings in the normal config blocks? and only put caching settings in the cache provider? This is the approach I preferred but I thought it was worth discussing.Roles are currently configured as a yaml list, but I'm tempted to make them a pipe delimited list of roles. I think it would read better, but seems strange when yaml supports lists directly.
Trace ID index had support for caching so I replicated it in this PR. Since this feature is known to be broken, I'm wondering if I should drop support until we invest the time to improve it.
New example config
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]