-
Notifications
You must be signed in to change notification settings - Fork 230
feat: schema cache to prevent thundering herd #1258
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
6254f64
b6e153d
e82a2a7
feed050
050ce90
2fe3347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| //! Cache the schema per database, so we don't have to fetch | ||
| //! it for each [`crate::backend::pool::Cluster`]. | ||
|
|
||
| use dashmap::DashMap; | ||
| use once_cell::sync::Lazy; | ||
| use std::sync::Arc; | ||
| use tokio::sync::Mutex; | ||
|
|
||
| use crate::backend::{Schema, Shard}; | ||
|
|
||
| type Entry = Arc<Mutex<Schema>>; | ||
|
|
||
| static CACHE: Lazy<SchemaCache> = Lazy::new(SchemaCache::default); | ||
|
|
||
| /// Schema cache. | ||
| #[derive(Debug, Default)] | ||
| pub(crate) struct SchemaCache { | ||
| // Database => shard => Schema | ||
| cache: DashMap<String, DashMap<usize, Entry>>, | ||
|
levkk marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| impl SchemaCache { | ||
| /// Get a schema entry from the cache or load it from | ||
| /// the server and store it in the cache. | ||
| /// | ||
| /// The loading is synchronized with a mutex, so only one user | ||
| /// can load a schema at a time, preventing a thundering herd situtation. | ||
| /// | ||
|
levkk marked this conversation as resolved.
Outdated
|
||
| pub(crate) async fn get(&self, shard: &Shard) -> Result<Schema, super::Error> { | ||
| // This is synchronized. | ||
|
levkk marked this conversation as resolved.
|
||
| let entry = self | ||
| .cache | ||
| .entry(shard.identifier().database.clone()) | ||
| .or_default() | ||
| .entry(shard.number()) | ||
| .or_default() | ||
| .clone(); | ||
|
|
||
| // This is syncrhonized too, | ||
| // so only one shard/user can fetch the schema at a time. | ||
| let mut guard = entry.lock().await; | ||
|
|
||
| if guard.is_loaded() { | ||
| return Ok(guard.clone()); | ||
| } | ||
|
|
||
| let schema = shard.fetch_schema().await?; | ||
|
|
||
| *guard = schema.clone(); | ||
|
Comment on lines
+43
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in theory here could be a tricky data race with on if for some reason we trigger The shutdown token would've be helpful, but this is called after the clear and launch, leaving this race window. There is another race that is not important I think: lock -> fetch_schema -> reload_databases -> set to old guard. But it's not a big issue since the guard will not point to the cache map entry anymore and that'd be a separate mutex.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I know what to do here. I'll add a global "config is reloading" signal that can cancel all of these long-running loops. I've been meaning to add something like this; making everything more deterministic.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might also have to be versioned too. Each config reload increments the config version so each
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. *race condition, not a data race.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this one is kinda tough. We may need to refactor the config system from an ArcSwap to a watch, or something. to ensure this works across the whole app. Right now, the race condition is real, but it may not have a real impact. i.e., if they race, they happen around the same time, so the schema is probably identical anyway. This was built specifically to busted when the user wants it to, e.g., they create a new table, then hit
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured it out. We should attach all config-scoped "globals" to the We should ban globals lol. Everything from now on should be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 2fe3347
levkk marked this conversation as resolved.
|
||
|
|
||
| Ok(schema) | ||
| } | ||
|
|
||
| /// Remove all entries from the schema cache. | ||
| pub(crate) fn clear(&self) { | ||
| self.cache.clear(); | ||
| } | ||
|
|
||
| /// Get a reference the the global schema cache. | ||
| pub(crate) fn global() -> &'static SchemaCache { | ||
| &CACHE | ||
| } | ||
|
levkk marked this conversation as resolved.
Outdated
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.