search

go-pkgz / lcw

Loading Cache Wrapper
https://go-pkgz.umputun.dev/lcw/
MIT License
20 stars 5 forks source link

Add RedisCache #5

Closed tyzhnenko closed 4 years ago

tyzhnenko commented 4 years ago

Issue #3

umputun commented 4 years ago

pls let me know as pr is good for review. I got a bunch of commits notifications and have no way to figure if this commit was final or not.

tyzhnenko commented 4 years ago

pls let me know as pr is good for review. I got a bunch of commits notifications and have no way to figure if this commit was final or not.

Ok. I'll ping you. I'm trying to understand how can I properly use BinaryMarshaler/Unmarshaler due redis client can't convert sizerString properly and want this interface.

tyzhnenko commented 4 years ago

pls let me know as pr is good for review. I got a bunch of commits notifications and have no way to figure if this commit was final or not.

I've updated pull request, fix readme and add tests. I have no idea how to add cache size to RedisCache in simple way that's why I skip size assertion in tests. I mean onEvicted hook at expired keys. Also I had to force convert string to sizedString due Redis client return value as only string

umputun commented 4 years ago

I have no idea how to add cache size to RedisCache in simple way that's why I skip size assertion in tests. I mean onEvicted hook at expired keys.

ok, it makes sense. I think you can drop this data-size support in case of redis completely. The original goal was to limit in-process memory consumption for the regular caches, but for redis the only size limit making some sense is the number of pairs we store. I mean, redis memory control is not really a concern we should address here.

tyzhnenko commented 4 years ago

I have no idea how to add cache size to RedisCache in simple way that's why I skip size assertion in tests. I mean onEvicted hook at expired keys.

ok, it makes sense. I think you can drop this data-size support in case of redis completely. The original goal was to limit in-process memory consumption for the regular caches, but for redis the only size limit making some sense is the number of pairs we store. I mean, redis memory control is not really a concern we should address here.

Can you make a review of the latest change set? I'd like to have full list of problems :) and then I'll go to fix them all at once

umputun commented 4 years ago

Added just a few minor things

tyzhnenko commented 4 years ago

Added just a few minor things

Please take a loop the latest changes. It looks like I made all necessary changes which were pointed in comments