Repository navigation
Existence cache remove callbacks - #1947
Conversation
| impl RemoveStateCallback<StoreKey<'static>> for RemoveItemCallbackHolder { | ||
| async fn callback(&self, key: &StoreKey<'static>) { | ||
| self.callback_fn.callback(key).await; |
There was a problem hiding this comment.
The RemoveItemCallback trait could use a module-level doc comment explaining its purpose (e.g., "For notifying caches of deletions from backing stores").
It's a pretty big piece of functionality.
There was a problem hiding this comment.
Yup, I'll add a few doc comments to the various new traits tomorrow.
| // as if it immediately expires them, we should only apply the remove callbacks | ||
| // afterwards. If this is None, we're not pausing; if it's Some it's the location to | ||
| // store them in temporarily | ||
| pause_remove_callbacks: Arc<Mutex<Option<Vec<StoreKey<'static>>>>>, |
There was a problem hiding this comment.
Are paused callbacks still flushed if an update fails mid way?
There was a problem hiding this comment.
Yes. The update is in two phases - inner store and then the existence cache - and both don't do any returns of errors until after the pause_remove_callbacks has been reset.
| // Bucket each key into the store that it belongs to. | ||
| keys.iter() | ||
| .cloned() | ||
| .cloned() |
There was a problem hiding this comment.
I'm not blaming you because it was here for almost two years. It think we could be more efficient than .cloned().enumerate(). The only reason I would even bring it up is the fact that shard_store really only matters for large workloads.
There was a problem hiding this comment.
Removed now the 'static fixes are done.
| async fn get_part( | ||
| self: Pin<&Self>, | ||
| key: StoreKey<'_>, | ||
| key: StoreKey<'static>, |
There was a problem hiding this comment.
Can you explain this change to me? No fault of your own, btw.
There was a problem hiding this comment.
I wasn’t suggesting you change. I wanted to know what was the rationale, mostly.
There was a problem hiding this comment.
It was always one of those changes that I wasn't entirely happy with, but I thought it was needed in order to support some part of the evicting map changes. It turns out that I think it was needed earlier on, but not anymore, so I've been able to reset this.
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
there's a lot of code to read and try out. I'll add more comments in a bit but wanted to get some comments to you now.
46c3c48 to
5217897
Compare
| # ThreadSanitizer to detect data races. Tests under tsan shouldn't be cached | ||
| # since they tend to produce irreproducible false negatives. | ||
| build:tsan --config=nightly -c dbg | ||
| build:tsan --config=nightly --config=debug |
This branch was previously deployed
2 participants
Description
There's an issue with the existence cache if one or more of it's underlying stores evicts items. Namely, when they do so, they don't tell anyone about it, and so the existence cache still says "yup, got that" and then clients will fail when they try to retrieve the item. This is believed (but not certain) as the source of the
not found in either fast or slow storeerrors that have been cropping up. This PR adds a callback mechanism for evictions so compound stores that need to know about their inner stores evicting things can get notified.Consequences of this change:
GrpcStoreas an internal store, as we don't get "I evicted this" feedback from those. These will now fail at runtime.EvictingMaptype has become more complicated to support using the borrow key there as the callback key.Type of change
Please delete options that aren't relevant.
not work as expected)
How Has This Been Tested?
bazel test //...Checklist
bazel test //...passes locallygit amendsee some docsThis change is