Skip to content
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

Cache cleanups #12807

Merged
merged 5 commits into from
Apr 1, 2019
Merged

Cache cleanups #12807

merged 5 commits into from
Apr 1, 2019

Conversation

zeebo
Copy link
Contributor

@zeebo zeebo commented Mar 21, 2019

This PR does some cleanups on the cache. Specifically

  • init and Free are unnecessary and can lead to data loss in some race conditions.
  • the cache *ring type only works with 16 partitions, so update the code and remove comments claiming otherwise.
  • fix up the benchmarks that attempt to use rings with different partitions.
  • refactor some types into their own files to keep single file length down.
  • create an export a type to allow consumers to detect when a write fails due to the cache being full.

Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely jubbly 👍

tsdb/tsm1/ring.go Show resolved Hide resolved
The code actually didn't work if 16 wasn't passed. Indeed, the
benchmarks weren't even working. Fix up all that, and reduce
the complexity some.
The storer interface isn't necessary if the init/Free logic is
removed, which is unnecessary in a world with only one shard.
Additionally, there were some cases where an init/Free call could
race and cause data loss in the cache. Not doing it at all fixes
all of those races.
This makes it easier and more robust to check if an error is due
to the cache memory limit being exceeded.
@e-dard
Copy link
Contributor

e-dard commented Apr 1, 2019

🍏

@zeebo zeebo merged commit a7c3f20 into master Apr 1, 2019
@zeebo zeebo deleted the jmw-cache-cleanups branch April 1, 2019 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants