store/cache: drop updates to existing items and dedupe get/set methods #1142
Conversation
abursavich
changed the title
|
Hmm, makes sense that we could do better when modifying an item but looking at the code and logically this should never happen AFAICT. It seems to me that the problem is that at first we check https://github.com/improbable-eng/thanos/blob/master/pkg/store/bucket.go#L1421 if there is an entry in the cache for that, and only then we add it https://github.com/improbable-eng/thanos/blob/master/pkg/store/bucket.go#L1493 to the cache. So it seems like some locking is missing there. What do you think? (cc @bwplotka) |
|
AFAICT, the "check" and "set" is not atomic so there may be many concurrent "checks" that miss the same items in the cache and then independently load and "set" them. |
|
Yes so that's my point - there's no reason to perform all of this work of removing and adding the same item again if we can put some locking around that part to make it atomic. What is your opinion on that? |
|
Wow, have you found the actual fix?? Awesome, cannot wait for some time to dive into it and review! ❤️ Thanks! |
|
I think the cache should support setting the same key multiple times. Since it goes out of its way to support concurrent access, I'm less inclined to accept an argument in favor of pushing the problem onto the caller. The lookup to remove the existing item is O(1), so it's not so bad. You can always add a singleflight in the caller later if you want to prevent duplicated work. |
|
Awesome spotting of this edge case. As you said, Get & Set are not atomic , but we missed that during implementation and testing. For the fix. You current approach of always removing makes sense. It is simple and effective, but not that efficient as we are removing from LRU something was added milisecond before. Let's see if that's the best what we can do. Let's agree on one invariant for this: I think this cache items are immutable. So there is nothing like old version of cache or new. Updating is not needed for the sake of it. I like the @GiedriusS idea of making it atomic, or use What about simple alternative to this - just try to GetPosting again just before Set in lock. O(1) as well. And this will ensure proper behaviour, but will still fetch postigns unncecessary. Your solution will also help if our invariant is wrong and cache is malformed. Thoughts @GiedriusS @abursavich ? Happy to discuss offline on slack as well (: |
When an item was successfully updated, metrics and cache size tracking added the new version of the item but did not remove the old version of the item, so it was over-counted. For example, with a fresh cache, if you set the same item m times with each version having entry size n, the cache would think it had m items with current size m*n instead of 1 item with current size n. If you then emptied the cache by evicting every item (only one), the cache would think it had m-1 items with current size (m-1)*n, instead of 0 items with current size 0. This would allow the cache to get stuck in an eviction loop which persists even after the cache has been emptied.
abursavich
changed the title
|
I changed it to make cache items immutable by aborting the set if the key already exists (instead of replacing the existing value). I also refactored the get and set methods to reduce duplication. |
|
Forgot about change log 🤦♂️ |
When an item is successfully updated, metrics and cache size tracking added the new version of the item but did not remove the old version of the item.
For example, with a fresh cache, if you set the same item
mtimes with each version having entry sizen, the cache would think it hadmitems with current sizem*ninstead of1item with current sizen.If you then emptied the cache by evicting every item (only one), the cache would think it had
m-1items with current size(m-1)*n, instead of0items with current size0.This would allow the cache to get stuck in an eviction loop which persists even after the cache has been emptied.
When an item is unsuccessfully updated in the cache because the new version is too big, the old version was left in the cache.
Changes
If an item exists in the cache, remove it and then re-add it. It would be possible to update the metrics in place, skipping what is counted as an eviction, but the code would be more complicated and brittle.
Verification
Added tests which previously failed and now pass.
Fixes #651