fix: support parallel script execution in pipelines #1304
Conversation
marcbachmann
force-pushed
the
support-parallel-script-execution-in-pipelines
branch
from
cfe6a65 to
227235a
Compare
| if ( | ||
| !script || | ||
| this.redis._addedScriptHashes[script.sha] || | ||
| scripts.includes(script) |
There was a problem hiding this comment.
We might want to replace the .includes with an object lookup or set.
A Set would automatically deduplicate it.
Maybe also use const scripts = {} and then Object.values(scripts) further down.
But the performance overhead is negligible as this operation only occurs every time the scripts cache is cleared. I'd rather deduplicate the execution on first load. With the current code the script gets loaded multiple times per for every pipeline created in parallel.
We could also change the behavior by assigning a promise and then reuse that in other requests.
Also doable in another PR if desired.
this.redis._addedScriptHashes[script.sha] = loadPromiseThe whole logic could also be moved into the custom command functions invoked in redis.sendCommand.
marcbachmann
force-pushed
the
support-parallel-script-execution-in-pipelines
branch
from
227235a to
fb80fae
Compare
marcbachmann
force-pushed
the
support-parallel-script-execution-in-pipelines
branch
from
fb80fae to
f03188d
Compare
## [4.24.3](v4.24.2...v4.24.3) (2021-03-21) ### Bug Fixes * support parallel script execution in pipelines ([#1304](#1304)) ([c917719](c917719))
|
🎉 This PR is included in version 4.24.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Awesome! Thanks for the contributions!
Functionality-wise that sounds good to me. Not sure if it worth the effort (ex considering the maintainability) though as it anyway only affects cases without cache. However, PR will be welcome if you see it as appropriate. Thanks again! |
marcbachmann
deleted the
support-parallel-script-execution-in-pipelines
branch
## [4.24.3](redis/ioredis@v4.24.2...v4.24.3) (2021-03-21) ### Bug Fixes * support parallel script execution in pipelines ([#1304](redis/ioredis#1304)) ([c917719](redis/ioredis@c917719))
Fixes #1303
Support parallel .multi script execution by delaying the assignment of the load confirmation.
btw. regarding the project setup: the package-lock.json links to
http://artprod.dev.bloomberg.com/artifactory/api/npm/npm-repos/as npm registry, which isn't publicly available. Thereforenpm installandnpm cifails locally. By replacing all the urls with the npm registry, the npm install works again.