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

Deleted entries should not be removed by expire eviction #219

Merged
merged 1 commit into from
May 6, 2020

Conversation

bachhh
Copy link
Contributor

@bachhh bachhh commented May 4, 2020

Hi, this is a small fix for #126,

I think the problem is that shard.delete() marks entries in queue as deleted ( by zero-ing all the 64 header bits for hash ), but removeOldestEntry is not checking for this case.

@codecov-io
Copy link

codecov-io commented May 4, 2020

Codecov Report

Merging #219 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   90.61%   90.64%   +0.02%     
==========================================
  Files          15       15              
  Lines         714      716       +2     
==========================================
+ Hits          647      649       +2     
  Misses         58       58              
  Partials        9        9              
Impacted Files Coverage Δ
shard.go 91.37% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37b9eb2...1363eac. Read the comment docs.

cristaloleg
cristaloleg previously approved these changes May 4, 2020
Copy link
Collaborator

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

sgtm

go.mod Outdated
@@ -1,3 +1,5 @@
module github.com/allegro/bigcache/v2
module bigcache
Copy link
Collaborator

Choose a reason for hiding this comment

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

you shouldn't change this file 😄

Copy link
Contributor Author

@bachhh bachhh May 5, 2020

Choose a reason for hiding this comment

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

oh sorry, I accidentally pushed local changes to my master
EDIT: reverted @cristaloleg

Copy link
Collaborator

@siennathesane siennathesane left a comment

Choose a reason for hiding this comment

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

I wasn't really sure what to do about this, but I love how straightforward the fix was. Thanks!

@siennathesane siennathesane merged commit d572104 into allegro:master May 6, 2020
@siennathesane
Copy link
Collaborator

Fixes #126.

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.

None yet

4 participants