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

add GetItems method #64

Merged
merged 2 commits into from
Dec 15, 2021
Merged

add GetItems method #64

merged 2 commits into from
Dec 15, 2021

Conversation

DoubleDi
Copy link
Contributor

@DoubleDi DoubleDi commented Dec 9, 2021

Hi! @ReneKroon !
Awesome cache!
Want to switch to yours from https://github.com/patrickmn/go-cache as yours has a size limit.

Can you accept a small PR which adds the GetItems method?

@ReneKroon
Copy link
Contributor

I think it's a good function. However i think it's more correct to call getItem on each individual key? That way all ttl related operations and notifications are respected.

@DoubleDi
Copy link
Contributor Author

DoubleDi commented Dec 9, 2021

@ReneKroon are you sure we should update all the TTL on this handler? getItem updates the TTL if skipTTLExtension is not true. IMHO GetItems() is a META handler, which does not indicate, that the element was actually retrieved. But will honour your decision on this one

Also planing on other PR:

  1. refactor mutex to rwmutex
  2. add a parameter which will specify how often to do the startExpirationProcessing loop. Currently it is hardcoded to 1 hour.

Will you support and accept those?

@ReneKroon
Copy link
Contributor

On the other PR proposals:

there is one outstanding PR that attempts this but fails (#58) on 'refactor mutex to rwmutex'. This is however very tricky since a 'get' can also modify TTL values. Note that @chenyahui made several other great performance improvements, but this one is way harder. But if you can make it work, go for it.

The startExpirationProcessing should be adjusted automatically when items with a lower TTL than the next processing loop are inserted. Did you find a case where this is not happening?

@DoubleDi
Copy link
Contributor Author

DoubleDi commented Dec 13, 2021

Got you! Will see what I can do.

About the expire loop timer. If the cache ttl is more than 1 hour (ex 24 hours), the loop will only run once every 24 hours. Not sure if that is a must to change that.

What about the current PR? Should I change to getItem or keep as is?

@ReneKroon
Copy link
Contributor

Please use the 'getItem' in the call so that the interface has consistent behavior.

@DoubleDi
Copy link
Contributor Author

Done

@ReneKroon ReneKroon merged commit f3ee70c into jellydator:master Dec 15, 2021
@ReneKroon
Copy link
Contributor

apologies for the crappy Travis build, it's having issues with a code coverage token on master branches in forks.

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.

2 participants