weyoss / redis-smq

A simple high-performance Redis message queue for Node.js.
MIT License
588 stars 64 forks source link

SET command throwing error on startup #22

Closed ghost closed 2 years ago

ghost commented 6 years ago
events.js:183
      throw er; // Unhandled 'error' event
      ^
ReplyError: ERR wrong number of arguments for 'set' command
    at parseError (C:\Users\212702834\Documents\Userdata\Practice\redis-message-queue\node_modules\redis-parser\lib\parser.js:193:12)
    at parseType (C:\Users\212702834\Documents\Userdata\Practice\redis-message-queue\node_modules\redis-parser\lib\parser.js:303:14)

Dependency Versions:

{
    "redis": "2.7.1",
    "redis-smq": "^1.0.20"
  }

Running on Windows 10, x64 Redis server version 2.4.5

ghost commented 6 years ago

RCA: SET command with multiple option is not supported Below Redis 2.6.12 Hence, seperation of redisClient.set(tmpLockKey, instanceId, 'NX', 'EX', 60, onGCTmpLock) needs to be done to

  1. redisClient.setnx
  2. redisClient.expire
weyoss commented 6 years ago

The minimal required Redis version is noted in README and it is 2.6.12

Versions below 2.6.12 are not supported.

Please upgrade your Redis installation to match the latest stable Redis version.

https://github.com/weyoss/redis-smq#installation

weyoss commented 6 years ago

I want to make some clarifications.

This message queue can not guaranty that it will work correctly if some it its requirements are not fulfilled.

Redis is a foundation for this message queue and it is being continuously updated, new bugs and errors are reported and fixed, new features are introduced.

Errors and bugs could appear just because the Redis version is different.

This package has been developed and tested on Redis version 2.6.12. Naturally, the support of new versions and backward compatibility support, down to version 2.6.12, is provided.

I hope that this makes sense to you and thank you for contributing to this package and trying to make it better.

I am open to any questions.

ghost commented 6 years ago

@weyoss I am currently operating with Redis server version 2.4.5 (00000000:0) which does not support multiple options in the SET command (Reference: https://redis.io/commands/set)

Hence, I added this PR with SETNX and EXPIRY.

This is not a breaking change, just backward compatibility. Other than that Redis SMQ is just perfect.

weyoss commented 6 years ago

Yes, I completely understand that your are running a Redis server version below 2.4.12 (2.4.5) and that your change just replaces set command with setnx and expire.

Based on the note:

Note: Since the SET command options can replace SETNX, SETEX, PSETEX, it is possible that in future versions of Redis these three commands will be deprecated and finally removed. - https://redis.io/commands/set

It is better to keep SET instead of replacing it with SETNX and PEXPIRE.

PS. SET with the NX and EX options is atomic.

ghost commented 6 years ago

Agreed. I anyway forked and modified for my use case. I don’t think it’ll be required by a greater audience though.

On Tue, 28 Aug 2018 at 7:44 PM, weyoss notifications@github.com wrote:

Yes, I completely understand that your are running a Redis server version below 2.4.12 (2.4.5) and that your change just replaces set command with setnx and expire.

Based on the note:

Note: Since the SET command options can replace SETNX, SETEX, PSETEX, it is possible that in future versions of Redis these three commands will be deprecated and finally removed. - https://redis.io/commands/set

It is better to keep SET instead of replacing it with SETNX and PEXPIRE.

PS. SET with the NX and EX options is atomic.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/weyoss/redis-smq/issues/22#issuecomment-416599938, or mute the thread https://github.com/notifications/unsubscribe-auth/AQ6FWYkfLd-bHUjVje92W_Q4wv2WbpWNks5uVVBNgaJpZM4WKyD7 .

-- Sent from Gmail Mobile