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

Use SCAN and add support for redis://:password@hostname:port?db=1 urls #1

Merged
merged 1 commit into from
Jul 20, 2016

Conversation

zph
Copy link
Contributor

@zph zph commented Jul 20, 2016

First off, thanks for writing this 👍 . It helps with migrating data when hosting provider locks down some access to system.

Here are my changes:

Use REDIS SCAN instead of KEYS

SCAN preferred to KEYS for production use:

Warning: consider KEYS as a command that should only be used in
production environments with extreme care. It may ruin performance when
it is executed against large databases. This command is intended for
debugging and special operations, such as changing your keyspace layout.
Don't use KEYS in your regular application code. If you're looking for a
way to find keys in a subset of your keyspace, consider using SCAN or
sets.

Optional URL Input Format

Adds support for redis:// type urls which are used in redis-cli, with
the optional extension of ?db=1 if db needs to be non-zero.

Adds specs to verify the behavior. Preferentially chooses that type of
link parsing if the url starts with redis:// prefix. Otherwise falls
through to existing behavior.

Uses INFO to get full key count. This is incorrect when selecting a subset of keys... but the alternate way doesn't give any prediction of status bar Total count because we're gradually iterating through keyspace rather than preloading, as is done with KEYS. Seems like an acceptable tradeoff for now, but I could remove it from the PR if desired.

Note

I brought in a different redis client in addition to current one because it cleanly supports the REDIS SCAN. This is a messy hack but it's working well enough for now.

Cheers,
Zander

Use REDIS SCAN instead of KEYS
==

SCAN preferred to KEYS for production use, thus syncing use:
- http://redis.io/commands/scan
- http://redis.io/commands/keys

```
Warning: consider KEYS as a command that should only be used in
production environments with extreme care. It may ruin performance when
it is executed against large databases. This command is intended for
debugging and special operations, such as changing your keyspace layout.
Don't use KEYS in your regular application code. If you're looking for a
way to find keys in a subset of your keyspace, consider using SCAN or
sets.
```

Optional URL Input Format
==
Adds support for redis:// type urls which are used in redis-cli, with
the optional extension of `?db=1` if db needs to be non-zero.

Adds specs to verify the behavior. Preferentially chooses that type of
link parsing if the url starts with redis:// prefix. Otherwise falls
through to existing behavior.
@zph
Copy link
Contributor Author

zph commented Jul 20, 2016

Also, what's the library LICENSE?

@adarqui
Copy link
Owner

adarqui commented Jul 20, 2016

Hey @zph!

Very happy to see people are actually using this (I had no idea), made my day :) Just built your fork and tested it out with ~1 million keys, worked great! Thanks so much for taking the time to make these changes. I will merge everything shortly. I will also add a LICENSE like you asked, and clean up a few things - as I haven't looked at this code in quite a while. I'll get back to you in a bit.

Thanks!

Andrew

@zph
Copy link
Contributor Author

zph commented Jul 20, 2016

@adarqui Awesome :). And better yet, it's the only way I've found to migrate this data with minimal service interruption. So it's super helpful!

Let me know if you'd like some of it tweaked or feel free to bring in those changes in whatever form you like.

@adarqui
Copy link
Owner

adarqui commented Jul 20, 2016

@adarqui Awesome :). And better yet, it's the only way I've found to migrate this data with minimal service interruption. So it's super helpful!

That's really cool!! Very happy to hear that. 👍

Let me know if you'd like some of it tweaked or feel free to bring in those changes in whatever form you like.

Yeah these will most likely be changes to the README, adding LICENSE/CONTRIBUTOR files, a few changes to the usage output etc.

Thanks again! Merging.

@adarqui adarqui merged commit 69e0934 into adarqui:master Jul 20, 2016
@zph zph deleted the use-redis-scan branch July 20, 2016 20:01
@zph
Copy link
Contributor Author

zph commented Jul 20, 2016

Thanks for putting your work out there 👍 It's greatly appreciated!

@adarqui
Copy link
Owner

adarqui commented Jul 20, 2016

Thanks for putting your work out there 👍 It's greatly appreciated!

Thanks!

Also, just letting you know that I made several changes (but no changes to your code, not needed). Mostly just cleaned a few things up, separated some code out into their own files, made the README.md more attractive, added the LICENSE, made the OP(codes) more descriptive, and added a CONTRIBUTORS file.

peace!!!

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