-
Notifications
You must be signed in to change notification settings - Fork 209
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
feat: metrics engine cache #1624
base: main
Are you sure you want to change the base?
Conversation
let metric_ids = samples | ||
.iter() | ||
.map(|s| MetricId(hash(s.name.as_slice()))) | ||
.collect::<Vec<_>>(); |
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.
We should avoid create new Vec in write path, it will hurt perf.
}); | ||
|
||
// 2.1 update cache metrics | ||
futures::future::join_all( |
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.
We should do this in MetricManager
module.
} | ||
} | ||
|
||
pub struct CacheManager { |
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.
For index cache manager, it will only cache series, metrics will be managed be metrics manager.
In this way, our code is more modular.
} | ||
|
||
struct TagIndexCache { | ||
cache: DashMap<SegmentDuration, ConcurrentTagKVMap>, |
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.
Two thoughts here:
- We should prefer std map over dash map, those third party may introduce unexpected bugs.
- For current segment cache, we can use an independent field to represent, so we can save one hashmap lookup.
Other questions we need to consider:
- How will you evict segment?
)) | ||
} | ||
|
||
async fn load_from_storage(&mut self) -> Result<()> { |
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.
The cache module shouldn't known how the persistence layer is implemented, we can move this method to IndexManager
.
Ok(()) | ||
} | ||
|
||
fn schema() -> Arc<Schema> { |
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.
Cache module has no schema, this field belong to IndexManager.
Rationale
#1623
Detailed Changes
TODO:
Test Plan
UT