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

Don't work methods that use KEYS command(ioredis) #245

Closed
witem opened this issue Jun 6, 2018 · 10 comments
Closed

Don't work methods that use KEYS command(ioredis) #245

witem opened this issue Jun 6, 2018 · 10 comments

Comments

@witem
Copy link
Contributor

witem commented Jun 6, 2018

Hi,
If I set in config keyPrefix. Some commands stop work(it described in ioredis docs: https://github.com/luin/ioredis/blob/master/README.md#transparent-key-prefixing)

Step to reproduce:

  • create NodeResque.Queue with not null options keyPrefix
  • create scheduled job
  • call await queue.timestamps()

Methods that stop working(maybe not all):

  • queue.timestamps()
  • queue.locks()
  • queue.stats()
  • scheduler.checkStuckWorkers()
@witem witem changed the title Don't work methods who use KEYS command(ioredis) Don't work methods who that KEYS command(ioredis) Jun 6, 2018
@witem witem changed the title Don't work methods who that KEYS command(ioredis) Don't work methods that use KEYS command(ioredis) Jun 6, 2018
@evantahler
Copy link
Member

Can you confirm that your keyPrefix does not have an special charecters?

As a note this might be conflicting with the node-resque "namespace" option:

@witem
Copy link
Contributor Author

witem commented Jun 8, 2018

Yes, it doesn't have any special charecters. And it not conflicting with namespace option.

I use keyPrefix: undefinedshmell

@usebaz
Copy link

usebaz commented Jul 10, 2018

@evantahler ping =)

@evantahler
Copy link
Member

In d3477e4 I've added a number of passing tests which demonstrate that (I think) the ioredis+prefix behavior is working properly.

Please fork this project, and add a failing test to this section which demonstrates the problem you are observing.

witem added a commit to witem/node-resque that referenced this issue Jul 11, 2018
@witem
Copy link
Contributor Author

witem commented Jul 11, 2018

Hi @evantahler
I add failed tests for queue.timestamps: witem@cad30e1

But I think also broken next methods:

  • queue.locks()
  • queue.stats()
  • scheduler.checkStuckWorkers()

@witem
Copy link
Contributor Author

witem commented Jul 18, 2018

Hi @evantahler
It's normal test case?

@evantahler
Copy link
Member

Thank you for your examples! That helped me to see that the issue was coming from any command which relies on redis.keys(), as other methods work OK with the keyPrefix.

It seems that supporting a keyPrefix with redis.keys() is explicitly not support by ioredis. It is mentioned in their readme, and in a number of issues:

The most clear response from @luin is

The first argument of KEYS command is a pattern rather than key name, so we don't prefix it with keyPrefix.

and I agree with him. I think that using keyPrefix in this way is dangerous at the command level. That is why we use redis "namespaces" instead in a way we have direct control over, which allows a similar result.

I would recommend either using databases within your redis server to arrange your data, rather thankeyPrefix going forward.

I will add a note to our readme that we do not support keyPrefix with ioredis (#256)

@witem
Copy link
Contributor Author

witem commented Jul 19, 2018

@evantahler maybe I can make PR, for allow make connection options namespace an array.

Example:

const connectionDetails = {
  namespace: ['prefix', 'resque'],
}
const connection = new NodeResque.Connection(connectionDetails)

And in method connection.key change unshift(https://github.com/taskrabbit/node-resque/blob/52c11a7932980ebfdcdfd2519d3b508f423680c6/lib/connection.js#L89) to:

if (Array.isArray(this.options.namespace)) {
  args.unshift(...this.options.namespace)
} else {
  args.unshift(this.options.namespace)
}

It's allow to use namespace more flexibly.

@evantahler
Copy link
Member

If you make your change non-breaking, ie: it works with both arrays and strings, that could work. Please be sure to test it!

@witem
Copy link
Contributor Author

witem commented Jul 25, 2018

Hi @evantahler
I add PR #258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants