fix(cluster): autopipeline when prefix is used #1335
Conversation
Previously the building of pipelines ignored the key prefix. It was possible that two keys, foo and bar, might be set into the same pipeline. However, after being prefixed by a configured "keyPrefix" value, they may no longer belong to the same pipeline. This led to the error: "All keys in the pipeline should belong to the same slots allocation group" This may fix redis#1264 and redis#1248.
| @@ -28,7 +28,10 @@ function findAutoPipeline( | |||
| } | |||
|
|
|||
| // We have slot information, we can improve routing by grouping slots served by the same subset of nodes | |||
| return client.slots[calculateSlot(args[0])].join(","); | |||
| const prefixedKey = client.options.keyPrefix | |||
There was a problem hiding this comment.
I'm working on the same codebase as the PR author using ioredis, so I decided to take a look. I'm only a bit familiar with ioredis
This approach seems reasonable.
Looking at lib/commander.ts
- When there is NO pipelining,
new Commandis used to transform args (e.g. add key prefixes) in its constructor - When there is pipelining, args are passed without any prefixing/transformation to the autoPipelining.ts - i.e. transforming here should be appropriate
// No auto pipeline, use regular command sending
if (!shouldUseAutoPipelining(this, commandName)) {
return this.sendCommand(
new Command(commandName, args, options, callback)
);
}
// Create a new pipeline and make sure it's scheduled
return executeWithAutoPipelining(this, commandName, args, callback);There was a problem hiding this comment.
Looking at lib/pipeline.ts for the "All the keys in a pipeline command should belong to the same slot" I don't see any issues - that checks if everything would go to the same shard using the prefixed keys (from Command.getKeys, Command.iterateKeys)
- e.g. if a pipeline only had one key, it would use the computed keys
this._iterateKeys((key) => options.keyPrefix + key);to determine the correct slots, and there would only be an issue if there were multiple keys and those would get sent to different slots if the prefix wasn't added.
I'd guess the new test case would be less prone to similar coincidences in the future if the unit test was called with more distinct keys
|
This can be closed - the changes in this were among the changes merged in #1393 |
When anyone has time, could this be closed so that people don't see stale PRs or worry about autopipelining needlessly. |
|
closing as requested |
Previously the building of pipelines ignored the key prefix.
It was possible that two keys, foo and bar, might be set into
the same pipeline. However, after being prefixed by a configured
"keyPrefix" value, they may no longer belong to the same
pipeline.
This led to the error:
"All keys in the pipeline should belong to the same slots
allocation group"
This may fix #1264 and #1248.