Support commands consisting of multiple words #363

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

dohse commented Jan 10, 2013

Currently redis multi-word commands like SCRIPT LOAD or CLIENT LIST are present, but do not work, because redis expects the words to arrive in individual fields.

This patch adds supports for these commands by registering the first word of such commands. The above can then be executed like:

redis.script('load', 'return 1');
redis.multi().script('load', 'return 1').exec(...);
redis.multi([['script', 'load', 'return 1']]).exec(...);

redis.client('list');
redis.multi().client('list').exec(...);
redis.multi([['client', 'list']]).exec(...);

As this patch only affects the initialisation of multi-word commands there are no performance impacts to expect.

The current solution has a small catch as prefixes that occur multiple times (like CLIENT from CLIENT LIST and CLIENT KILL) gets added multiple times and therefore overwritten. This has currently no impacts, but might yield unexpected results in future modifications. I could change that.

There is an alternative approach in #181. As this one modifies late stage command processing, it might have a performance impact. The command syntax is different there:

redis['script load']('return 1');
redis.multi()['script load']('return 1').exec(...);
redis.multi([['script load', 'return 1']]).exec(...);

Ready to merge checklist

  • test(s) in test.js
  • tests will fail without the PR, but succeed once applied
  • docs, if applicable
  • merges cleanly
  • coding style (4-space indents, looks similar to other code)
Contributor

DTrejo commented Feb 18, 2013

Could you add a test for a multi-word command?
Also, maybe a one-liner example on using multi-word commands in the docs.

Closing #181 in favor of this PR.

Thanks,
D

@DTrejo DTrejo referenced this pull request Feb 18, 2013

Closed

send_command() patch #181

Contributor

dohse commented Feb 20, 2013

Added test and documentation. On my machine the tests currently do not fly, but SCRIPT_LOAD does.

> node test.js
Connected to 127.0.0.1:6379, Redis server version 2.6.10

Using reply parser javascript
- flushdb: 1 ms
- incr: 1 ms
- multi_1:Uncaught exception: Error: Error: EXECABORT Transaction discarded because of previous errors.
    at Command.Multi.exec [as callback] (/home/vagrant/work/node_redis/index.js:1030:23)
    at RedisClient.return_error (/home/vagrant/work/node_redis/index.js:500:25)
    at ReplyParser.RedisClient.init_parser (/home/vagrant/work/node_redis/index.js:260:14)
    at ReplyParser.EventEmitter.emit (events.js:96:17)
    at ReplyParser.send_error (/home/vagrant/work/node_redis/lib/parser/javascript.js:293:10)
    at ReplyParser.execute (/home/vagrant/work/node_redis/lib/parser/javascript.js:176:22)
    at RedisClient.on_data (/home/vagrant/work/node_redis/index.js:476:27)
    at Socket.<anonymous> (/home/vagrant/work/node_redis/index.js:79:14)
    at Socket.EventEmitter.emit (events.js:96:17)
    at TCP.onread (net.js:397:14)

assert.js:102
  throw new assert.AssertionError({
        ^
AssertionError: true == false
    at process.<anonymous> (/home/vagrant/work/node_redis/test.js:1700:12)
    at process.EventEmitter.emit (events.js:96:17)
    at process.startup.processKillAndExit.process.exit (node.js:436:17)
    at process.<anonymous> (/home/vagrant/work/node_redis/test.js:1695:13)
    at process.EventEmitter.emit (events.js:126:20)
Owner

brycebaril commented Feb 20, 2013

You can ignore these test failures, they are for version changes in Redis and there is a PR #378 to fix them. Feel free to just locally comment MULTI_1, MULTI_2 and possibly KEYS.

Contributor

dohse commented Feb 21, 2013

I did that and the script load test works fine

@DTrejo DTrejo closed this in f0ae664 Feb 24, 2013

Contributor

DTrejo commented Feb 24, 2013

Thank you very much!
D

@DTrejo DTrejo referenced this pull request Feb 24, 2013

Closed

SCRIPT command corrections #366

fwiw, the comments didn't quite work for me when trying to do a "client list"

  • client.multi().script('load', 'return 1').exec(...);
  • client.multi([['script', 'load', 'return 1']]).exec(...);

The first one was just invalid, and the second one only worked when I got rid of the 'return 1'. These are also in the main readme and might benefit from correction.

Owner

dohse replied Mar 14, 2013

I amended the tests with these two cases (in 99f3afb) and they seem to work. Do these tests work at your machine? (I needed to comment out tests.FWD_ERRORS_1) Do you know what is wrong with the docs?

The syntax of the first gives:

TypeError: Object # has no method 'script'

And the second complained:

[ 'ERR Syntax error, try CLIENT (LIST | KILL ip:port)' ]

when it was like myclient.multi([['client', 'list', 'return 1']]).exec(

Deleting the 'return 1' did the trick for client list.

Maybe this is obvious.. I'm a newbie.

Owner

dohse replied Mar 14, 2013

Are you using the most recent version of node-redis? Did you run the tests with node test.js and had a line like

- script_load: 2 ms

in the output?

Owner

dohse replied Mar 14, 2013

Owner

dohse replied Mar 14, 2013

@dohse dohse referenced this pull request Mar 14, 2013

Merged

Enable client command #401

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