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

redis-memoizer package on npm #2

Open
STRML opened this issue Jul 14, 2016 · 10 comments
Open

redis-memoizer package on npm #2

STRML opened this issue Jul 14, 2016 · 10 comments

Comments

@STRML
Copy link
Collaborator

STRML commented Jul 14, 2016

This library was very useful to me in early development, and I've iterated on it significantly and done a few dozen releases.

https://github.com/BitMEX/redis-memoizer

I'm happy to publish it as redis-memoizer2 on npm, but if you're happy with the changes it would be great to have the name and continue the new version as the main project.

@willfarrell willfarrell mentioned this issue Jan 10, 2017
@rakeshpai
Copy link
Member

Sorry for the delay. Are you still up for this? I'd love it if you could send a PR, with some details about what's changed.

@rakeshpai
Copy link
Member

Just added you as a collaborator. Feel free to add your changes and merge them to this repo as you prefer, and let me know. I'll then publish it to npm. Thanks in advance!

@rakeshpai
Copy link
Member

Sorry for not waiting longer for a response, but I needed to get this going quickly. I've now rolled out a major version change with many (but not all) of the features you've got in your fork. I'd love to talk about how we can merge in the remainder of your changes.

@STRML
Copy link
Collaborator Author

STRML commented Nov 21, 2017

Yeah, great, I just saw that. What else are you interested in merging?

@rakeshpai
Copy link
Member

Thanks for responding.

I think the gzip thing you're doing is very interesting, so that's one thing worth looking at. I know that it isn't trivial to get data about such things, but do you have any data about if gzip actually helps? Any idea about the performance cost of the gzip? (Anecdotal information is fine.)

Your fork memoizes errors, and has the memoize_errors_when handler, which is another difference. This module currently doesn't memoize errors at all (it used to). I'm on the fence about whether caching errors is necessarily a good idea. However, since you've obviously given this more thought than I have, I'd love to know what you think.

One other difference is that you're explicitly handling/casting dates, which is also a great idea. I think that one can be stolen without needing much thought. ;)

I think other than that (and the obvious promise thing), our modules are now pretty close to each other. .name here does what .memoize_key_namespace does in your module, and .lockTimeout is similar to your .lookup_timeout.

What I'm thinking is, rather than maintaining a fork we could focus on one module and make it suit all our use-cases. It might not be possible for you to move over to this module again, now that you have a fork, of course, but it's a good aim to have. :)

@STRML
Copy link
Collaborator Author

STRML commented Nov 21, 2017

Sure - I memoize errors generally just to catch bad requests; we get a lot of traffic that gets validated at a lower layer in the stack and I want to avoid even sending that req if I know it's going to get rejected.

As for gzip, I forget when I measured it but it did indeed improve response time due to far fewer packets on the wire. We memoize a lot of JSON which compresses fantastically.

Dates are a practical issue, as is gzip.

Would be happy to merge them more; feel free to bring in more features and if you have any questions or next steps I'll take a look and open a PR or two. I can then look at migrating my use case back onto mainline.

@rakeshpai
Copy link
Member

Regarding the errors, wouldn't it be possible (maybe more appropriate as well), to use the tll function thing we have? A 4xx series http error isn't a JS error, ie. it's a successful network roundtrip with a valid response. You could set the ttl for such responses dynamically using the ttl function, isn't it? Or am I missing something?

I think it's worth distinguishing between proper JS execution errors and http errors, which aren't language errors. I think http errors are perfectly valid to cache, and I think the current setup allows for that. In any case, IIUC, I think we're agreeing that language errors shouldn't be cached.

+1 about gzip and dates. I'll play around with them and see I can get some perf numbers.

@STRML
Copy link
Collaborator Author

STRML commented Nov 21, 2017

Sure - in our code we throw that as an HTTPError, and use memoize_errors_when to pick those out specifically and memoize them. Not everyone throws errors on non-200s, but we do. Regardless, the hook offers increased flexibility.

@0x80
Copy link

0x80 commented Mar 9, 2020

@STRML @rakeshpai So judging from how old this conversation is people are advised to just use redis-memoizer2 now?

Are there any downsides to using that library over this one?

There are several repos that seem to have been forked from this one still using the name redis-memoizer and also not explaining their relationship to this repo. So it's all a bit confusing.

@0x80
Copy link

0x80 commented Mar 9, 2020

So redis-memoizer2 doesn't exist, nor does https://github.com/BitMEX/redis-memoizer. So things were merged after all? 🤔

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

3 participants