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

Activity

1. witem

   changed the title [-]Don't work methods who use KEYS command(ioredis)[/-] [+]Don't work methods who that KEYS command(ioredis)[/+] on Jun 6, 2018
2. witem

   changed the title [-]Don't work methods who that KEYS command(ioredis)[/-] [+]Don't work methods that use KEYS command(ioredis)[/+] on Jun 6, 2018
3. evantahler commented on Jun 8, 2018

   

   evantahler

   on Jun 8, 2018

   Member

   More actions

   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:

   • defined here https://github.com/taskrabbit/node-resque/blob/master/lib/connection.js#L85-L91
   • namespace defined here: https://github.com/taskrabbit/node-resque/blob/master/lib/connection.js#L12
4. evantahler

   added

   bug

   waiting-on-poster

   on Jun 8, 2018
5. witem commented on Jun 8, 2018

   

   witem

   on Jun 8, 2018

   ContributorAuthor

   More actions

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

   I use keyPrefix: undefinedshmell
6. usebaz commented on Jul 10, 2018

   

   usebaz

   on Jul 10, 2018

   More actions

   @evantahler ping =)
7. evantahler commented on Jul 10, 2018

   

   evantahler

   on Jul 10, 2018

   Member

   More actions

   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.
8. witem

   added a commit that references this issue on Jul 11, 2018

   master | add failed test for issue actionhero#245

   ...

   cad30e1
9. witem commented on Jul 11, 2018

   

   witem

   on Jul 11, 2018

   ContributorAuthor

   More actions

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

   But I think also broken next methods:

   • queue.locks()
   • queue.stats()
   • scheduler.checkStuckWorkers()
10. witem commented on Jul 18, 2018

    

    witem

    on Jul 18, 2018

    ContributorAuthor

    More actions

    Hi @evantahler
    It's normal test case?
11. evantahler commented on Jul 18, 2018

    

    evantahler

    on Jul 18, 2018

    Member

    More actions

    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:

    • using the results of "KEYS" when a keyPrefix option is being used redis/ioredis#381
    • keyPrefix in 'redis.keys' command redis/ioredis#254
    • keyPrefix vs prefix redis/ioredis#359
    • Key prefixing with blpop returns prefixed key redis/ioredis#325

    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)
12. evantahler

    closed this as completedon Jul 18, 2018
13. witem commented on Jul 19, 2018

    

    witem

    on Jul 19, 2018

    ContributorAuthor

    More actions

    @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.
14. evantahler commented on Jul 19, 2018

    

    evantahler

    on Jul 19, 2018

    Member

    More actions

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

    

    witem

    on Jul 25, 2018

    ContributorAuthor

    More actions

    Hi @evantahler
    I add PR #258