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 support for redis namespaces #41

Closed
wants to merge 1 commit into from

Conversation

Demannu
Copy link

@Demannu Demannu commented Oct 31, 2023

Why?

I'm exploring running screeps in a distributed way and separating each instance while maintain a cluster deployed redis is super useful. I had made this fork for myself to enable prefixing the redis queries and it's worked fairly well. I thought I'd share it with the community

How?

Add a new dependency ioredis which has native Redis client class support while also adding some optimization and most importantly key prefixing

This allows a drop-in replacement which also honors a new configuration option namespace which is then translated to ${namespace}: and saved to redis.config.keyPrefix. The remaining calls are natively compatible and had no known issues.

@Demannu Demannu force-pushed the support-namespaces branch 3 times, most recently from 070e7cc to f72147a Compare October 31, 2023 01:26
@AlinaNova21
Copy link
Member

This seems like an interesting idea, my only concern is it is a breaking change, as if I'm understanding this correctly, it will require a reset and not work with current databases. Would there be an easy change to allow existing databases to work? Maybe default namespace to ""?

@Demannu
Copy link
Author

Demannu commented Oct 31, 2023

It would be a breaking change, you're correct. I hadn't considered that.

Defaulting to an empty string should work 👍

@AlinaNova21
Copy link
Member

Was just looking into it a bit more, it seems that this wouldn't be safe to use, ioredis does not do any rewriting to work around calls such as flushdb, meaning everytime a resetAllData is called, or even importMap, it would result in clearing the enttire redis instance. Not just the prefixes. I found a few issues mentioning this along with a warning in the readme.
redis/ioredis#239 (comment)
redis/ioredis#325 (comment)

An alternative, which I have actually used in the past, is pass in the db number to redis. Which screepsmod-mongo already supports via config.redis.database or by passing a redis url containing one.

@AlinaNova21 AlinaNova21 closed this Nov 8, 2023
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