Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support commands consisting of multiple words #363

Closed
wants to merge 1 commit into from

4 participants

Jonas Dohse David Trejo Bryce Baril breville
Jonas Dohse

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)
David Trejo
Collaborator

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

David Trejo DTrejo referenced this pull request
Closed

send_command() patch #181

Jonas Dohse

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)
Bryce Baril
Collaborator

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.

Jonas Dohse

I did that and the script load test works fine

David Trejo DTrejo closed this pull request from a commit
Jonas Dohse dohse Use first word of multi word commands
Close #363.

Signed-off-by: DTrejo <david.daniel.trejo@gmail.com>
f0ae664
David Trejo DTrejo closed this in f0ae664
David Trejo
Collaborator

Thank you very much!
D

David Trejo DTrejo referenced this pull request
Closed

SCRIPT command corrections #366

breville

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.

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.

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?

Jonas Dohse dohse referenced this pull request
Merged

Enable client command #401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 20, 2013
  1. Jonas Dohse
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 1 deletion.
  1. +9 −0 README.md
  2. +3 −1 index.js
  3. +11 −0 test.js
9 README.md
View
@@ -543,6 +543,15 @@ This will display:
`send command` is data sent into Redis and `on_data` is data received from Redis.
+## Multi-word commands
+
+To execute redis multi-word commands like `SCRIPT LOAD` or `CLIENT LIST` pass
+the second word as first parameter:
+
+ client.script('load', 'return 1');
+ client.multi().script('load', 'return 1').exec(...);
+ client.multi([['script', 'load', 'return 1']]).exec(...);
+
## client.send_command(command_name, args, callback)
Used internally to send commands to Redis. For convenience, nearly all commands that are published on the Redis
4 index.js
View
@@ -865,7 +865,9 @@ commands = set_union(["get", "set", "setnx", "setex", "append", "strlen", "del",
"persist", "slaveof", "debug", "config", "subscribe", "unsubscribe", "psubscribe", "punsubscribe", "publish", "watch", "unwatch", "cluster",
"restore", "migrate", "dump", "object", "client", "eval", "evalsha"], require("./lib/commands"));
-commands.forEach(function (command) {
+commands.forEach(function (fullCommand) {
+ var command = fullCommand.split(' ')[0];
+
RedisClient.prototype[command] = function (args, callback) {
if (Array.isArray(args) && typeof callback === "function") {
return this.send_command(command, args, callback);
11 test.js
View
@@ -369,6 +369,17 @@ tests.EVAL_1 = function () {
}
};
+tests.SCRIPT_LOAD = function() {
+ var name = "SCRIPT_LOAD",
+ command = "return 1",
+ commandSha = crypto.createHash('sha1').update(command).digest('hex');
+
+ bclient.script("load", command, function(err, result) {
+ assert.strictEqual(result.toString(), commandSha);
+ next(name);
+ });
+};
+
tests.WATCH_MULTI = function () {
var name = 'WATCH_MULTI', multi;
Something went wrong with that request. Please try again.