Refactor DeleteExpired for safer behavior #60
Conversation
|
it might also fix #59 |
AxelPrel
force-pushed
the
delete-expired-fixes
branch
from
3fcf44b to
9289779
Compare
- make sure that expManager queue is not empty before popping, which can happen if an explicit delete happens while DeleteExpired releases the lock. - evict() now returns shouldContinue instead of a shouldBreak to simplify the loop.
AxelPrel
force-pushed
the
delete-expired-fixes
branch
from
9289779 to
a613de3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 11
Lines 527 523 -4
=========================================
- Hits 527 523 -4 ☔ View full report in Codecov by Sentry. |
cache.go
Outdated
| //Print tries to make a readable log of the item | ||
|
|
| return false | ||
| } | ||
| key, expiration := c.expManager.pop() | ||
| if nowFunc().After(expiration) { |
There was a problem hiding this comment.
I think better to use expired than this because a Expired method already has checking expiration logic.
There was a problem hiding this comment.
This code was meant to avoid an extra cache.Get(). Also, if the cache already had deleted the item, Expired() was not callable. However, your approach is more maintainable, since if you edit the way Expired() works you need to edit this code as well. So what should I do about this change ?
|
I don't think this would fix #59 as there are no limitation or scale down operations on capacity of |
make sure that expManager queue is not empty before popping, which can happen if an explicit delete happens while DeleteExpired releases the lock.
evict() now returns shouldContinue instead of a shouldBreak to simplify the loop.