LRU cache ensureFits() tweaks WRT deadlocking #796
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
January 30, 2019 13:33
This could potentially spin forever if the size of that item is bigger than the max size so add this special case to handle it instead of spinning forever. In this case, the oldest one will not be removed in this function but later an item might be evicted when we will add an item to the LRU with c.lru.Add() so it will be OK either way -- just that we will not spin here forever.
* ensureFits() now returns true if the passed items will fit in the cache * Wire up the results to a new counter metric thanos_store_index_cache_items_overflowed_total * Add cache tests to check the edge cases
bwplotka
reviewed
Feb 1, 2019
pkg/store/cache.go
Outdated
| @@ -127,7 +139,11 @@ func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { | |||
| c.mtx.Lock() | |||
| defer c.mtx.Unlock() | |||
|
|
|||
| c.ensureFits(v) | |||
| fits := c.ensureFits(v) | |||
| if !fits { | |||
| func (c *indexCache) ensureFits(b []byte) bool { | ||
| if uint64(len(b)) > c.maxSize { | ||
| return false | ||
| } | ||
| for c.curSize+uint64(len(b)) > c.maxSize { | ||
| c.lru.RemoveOldest() |
There was a problem hiding this comment.
I think we could do more aggressive removal if the slice is big enough.
There was a problem hiding this comment.
Yes, but how the implementation would be like? I have two ideas:
- If the slice size exceeds a certain threshold then we could remove 2 or more items at a time
- Get all keys, calculate their sizes, and remove the top X items to make sure there's enough size for the item
b?
Either way, the scope would definitely blow up since I do not think it is worth making these kinds of changes without scrupulous benchmarking.
There was a problem hiding this comment.
Either way, maybe lets leave it for another day? I am mainly interested now in making sure that the loop doesn't go forever
added 2 commits
February 1, 2019 15:42
No need to create an extra variable to hold a boolean.
Inc() is specifically designed for this case so use it instead of Add(1).
GiedriusS
changed the title
bwplotka
approved these changes
Feb 1, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #651. An initial attempt to fix this because I am always nervous that some person could unintentionally make our Thanos Store instance hang even if the index cache is quite big :(
Changes
thanos_store_index_cache_items_overflowed_totalmetric added which shows how many times we have tried to add an item to the cache which was simply too big according to the--index-cache-sizeparameter.ensureFits()now returns false when we cannot possibly ensure that the passed slice could fit into the cache. The original problem still exists in some form thatsetPostingsorsetSeriescould block for a longer time if there are a lot of small items in the cache however at least it shouldn't dead-lock.Verification
Wrote unit tests for it and they pass. It is kind of hard to reproduce this unfortunately in a live environment with the changes but I will see what I can do.