-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Pipeline-based script loading and retries #1499
Conversation
c6d0df3
to
19d23ea
Compare
lib/script.ts
Outdated
|
||
// Standalone mode (regular) | ||
function usingStandalone(script, redis, args, options, callback) { | ||
if (redis._addedScriptHashes[script.sha]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing those decisions in here, we could move it into the command.toWritable
function.
But we don't have any stream context there. https://github.com/luin/ioredis/blob/b8177479c348aa4bbd467fa944d61fe9b35aec19/lib/redis/index.ts#L790
If we would pass in the stream to the toWritable function, we could maintain the script cache per cluster node or socket instead on the redis instance.
@luin what do you think about those changes to make the scripts more reliable? But in general it completely eliminates all caching issues as we just load the script once with every new connection. Previously it resulted in quite a lot of NOSCRIPT errors. I think this drawback is good enough for most cases. |
|
||
// Resend the same custom evalsha command that gets transformed to an eval | ||
// in case it's not loaded yet on the connectionDo an eval as fallback, redis will hash and load it | ||
const resend = new this.Command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove this fallback if order of delivery needs to be guaranteed.
It will still clear the cache for the next command that gets sent to redis.
https://github.com/luin/ioredis/pull/1499/files#diff-3618aaced6e025d978d2cdde354e756687916385e9024868d0ad3fff887c55c6R24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking to include this breaking change in the next major version #1504 so we can just retry custom commands in pipeline as well.
lib/redis/index.ts
Outdated
|
||
if (stream) { | ||
if (stream.isPipeline) | ||
stream.write(command.toWritable(stream.destination.redis.stream)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the pipeline we could move the buffer logic into the Redis class itself.
Maybe even persist the pipeline itself in the queue instead of calling sendCommand that often.
Hey @marcbachmann 👋, Sorry for the delay. I'm planning v5 in #1504. So do you mind change the target branch of this PR to |
2dbf3da
to
7c4b38f
Compare
7c4b38f
to
4352875
Compare
@luin I've rebased this. |
99d9371
to
64a50dc
Compare
Awesome! Nice and clean improvements! |
This PR builds on #1497 (first three comments)
The last two commits are relevant for the new script load behavior.
Changelog