-
-
Notifications
You must be signed in to change notification settings - Fork 5
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 RedisCache #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the PR. In addition to things above it has two more parts missing:
- Tests
- Change in README
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. |
I've updated pull request, fix readme and add tests. |
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 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thx!
Issue #3