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

Fix more edge cases in autopipelining when using redis cluster #1393

Closed

Conversation

TysonAndre
Copy link
Collaborator

Fixes #1392

This is a superset of the fixes in #1391

The other issue this PR fixes was that this was passing an array instead of a string to
cluster-key-slot when args[0] was an array

Handle pathological cases such as redis.get([], ['key1', 'key2'])
by flattening the entire args array.

Prior to this PR, in the below example, the array was hashed instead of the first string in the array,
meaning that 'v{profile}1' and 'b' were incorrectly put in the same pipeline

const IORedis = require('./built');
const process = require('process');
async function main() {
    // Using instructions from https://redis.io/topics/cluster-tutorial
    // redis-cli --cluster create 127.0.0.1:7000 127.0.0.1:7001 127.0.0.1:7002 --cluster-replicas 0
    const redis = new IORedis.Cluster([{ host: '127.0.0.1', port:7000 }], { enableAutoPipelining: true });
    const results = await Promise.all([
      redis.mget(['v{profile}1', 'v{profile}2', 'v{profile}3', 'v{profile}4']),
      redis.get('b'),
    ]);
    console.log(results);
    process.exit(0);
}
main();

Fixes redis#1392

The issue was that this was passing an array instead of a string to
cluster-key-slot when `args[0]` was an array

Handle pathological cases such as redis.get([], ['key1', 'key2'])
by flattening the entire args array.
@TysonAndre
Copy link
Collaborator Author

TysonAndre commented Jul 28, 2021

Checking node_modules/redis-commands/commands.json for keyStart == keyStop may be a cheap way to detect commands such as get which have only one key without needing to flatten a potentially large array, but it seems like the redis-commands API doesn't treat commands.json as part of the public API. Not sure if that would be micro-optimizing too much.

Alternately, the multiple key check could just be omitted, since that would be a redis cluster sharding error anyway already

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