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

Extracting caching layer #31

Closed
cameron-martin opened this issue Aug 10, 2014 · 2 comments
Closed

Extracting caching layer #31

cameron-martin opened this issue Aug 10, 2014 · 2 comments

Comments

@cameron-martin
Copy link
Contributor

Would you accept a PR for extracting the redis caching into it's own gem, as a pluggable layer?

It doesn't seem right having redis as a hard-coded dependency of this gem, and doing the above would open up the possibility of using other caching strategies.

I also wanted to create something to automatically combine the requests which take multiple ids occuring in a certain timeframe, and split them back up again, transparently to the api consumer. Adding the above infrastructure would make this easy.

I wanted to get your approval before doing it, since this is a non-trivial change. Let me know what you think.

@intinig
Copy link
Contributor

intinig commented Aug 11, 2014

For now I don't mind having redis as a dependency, since it will install even if you have no redis on your box.

An extraction could be a good thing in the future but for now it seems ovecomplication, also because it's written in a way that already supports eventual new caching layers.

Regarding the request splitting and merging I don't think it's useful. Riot will increase rate limits without problems if you need them, and this would be complicating the library a little bit too much.

@cameron-martin
Copy link
Contributor Author

Yeah fair enough, I did some memory profiling and loading the redis gem only uses 5MB of extra memory. For this amount of memory savings, its not worth it.

Regarding the request splitting, I wasn't proposing adding it to this library, but implementing it in another gem in the same way that the caching layer would. However, I agree that this would probably be pointless; ideas always seem better when you first come up with them.

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

No branches or pull requests

2 participants