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.

Sign up for GitHub

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

Jump to bottom

Chaining custom commands in cluster mode #536

Closed
manast opened this issue Oct 25, 2017 · 31 comments · Fixed by #1201
Closed

Chaining custom commands in cluster mode #536

manast opened this issue Oct 25, 2017 · 31 comments · Fixed by #1201
Labels
pinned released

Comments

@manast
Copy link

manast commented Oct 25, 2017

According to the documentation "Chaining custom commands in the pipeline is not supported in Cluster mode.".
I am wondering, is this a limitation of ioredis or of redis itself? and do you have any information or link on why this limitation exists?
@manast manast mentioned this issue Oct 25, 2017
Open
@stale
Copy link

stale bot commented Nov 24, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Nov 24, 2017
@manast
Copy link
Author

manast commented Nov 24, 2017

@luin please, could you be so kind to share any insight you have in this issue?

@stale stale bot removed the wontfix label Nov 24, 2017
@luin
Copy link
Collaborator

luin commented Nov 24, 2017

Sorry for the late response.

That's not a limitation of Redis. It's just not easy to support chaining custom commands in the cluster setup: ioredis always try evalsha first when sending custom commands, and when the command fails, eval will be sent instead. However, when it comes to pipelines, to make sure all commands in a pipeline will be received by Redis in order, ioredis sends script exists command to check if all lua scripts have loaded in the target redis node and will load the scripts which haven't loaded before sending the pipeline.

In the cluster setup, the nodes may change (e.g. failover happens) during the process of loading scripts, so it's hard to make sure that the scripts are loaded to the correct redis node. That's why this feature haven't been implemented.

@manast
Copy link
Author

manast commented Nov 26, 2017 •
edited
Loading

thanks for the throughout explanation. When calling defineCommand, why can't we just call SCRIPT LOAD and then after that just assume that the command is available?

@luin
Copy link
Collaborator

luin commented Nov 26, 2017 •
edited
Loading

@manast Script cache may be cleared by the SCRIPT FLUSH command. Despite that, calling SCRIPT LOAD when defining commands indeed solves the problem (we also need to resend all SCRIPT LOAD again when a new connection has been made). Good idea! 👍

@stale
Copy link

stale bot commented Dec 26, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Dec 26, 2017
@manast
Copy link
Author

manast commented Dec 27, 2017

Bump! :)

@stale stale bot removed the wontfix label Dec 27, 2017
@stale
Copy link

stale bot commented Jan 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Jan 26, 2018
@btd
Copy link

btd commented Jan 26, 2018

Bump

@stale stale bot removed the wontfix label Jan 26, 2018
@stale
Copy link

stale bot commented Feb 25, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Feb 25, 2018
@manast
Copy link
Author

manast commented Feb 25, 2018

Bump!

@stale stale bot removed the wontfix label Feb 25, 2018
@stale
Copy link

stale bot commented Mar 27, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Mar 27, 2018
@btd
Copy link

btd commented Mar 27, 2018

Bump

@stale stale bot removed the wontfix label Mar 27, 2018
@stale
Copy link

stale bot commented Apr 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

@stale stale bot added the wontfix label Apr 26, 2018
@manast
Copy link
Author

manast commented Apr 26, 2018

Bump because it is an important issue!

@stale stale bot removed the wontfix label Apr 26, 2018
@Rush
Copy link

Rush commented May 7, 2018

Bump - interested in it as well.

@luin luin added the pinned label May 8, 2018
@firedfox
Copy link

Bump because we are using more Redis clusters in recent projects. This problem is getting more serious.

@ajwootto
Copy link

Also running into this.

@btzsoft
Copy link

btzsoft commented Apr 16, 2019

we have the same issue.

@botzill
Copy link

botzill commented Apr 16, 2019

Any updates on this issue? @luin

Thx.

@jl9404
Copy link

jl9404 commented Apr 29, 2019 •
edited
Loading

@manast Script cache may be cleared by the SCRIPT FLUSH command. Despite that, calling SCRIPT LOAD when defining commands indeed solves the problem (we also need to resend all SCRIPT LOAD again when a new connection has been made). Good idea!

@luin wondering if we could always send SCRIPT LOAD to the pipeline so we can EVALSHA safely? Thx.

@r3nat r3nat mentioned this issue May 23, 2019
Closed
@RPallas92
Copy link

Same issue here

@andreimc
Copy link

andreimc commented Sep 5, 2019

same issue here

1 similar comment
@kaikash
Copy link

kaikash commented Sep 13, 2019

same issue here

@rozquit
Copy link

rozquit commented Sep 17, 2019

Any updates on this issue? @luin

@NivLipetz
Copy link

Hi @luin , are there any plans for this issue to be fixed or is it a lost cause?
Thanks!

@shyimo
Copy link

shyimo commented Jul 23, 2020

Hi @luin , any plans about adding this ?
we are now fully use Redis cluster and it's getting more serious now

Rua-Yuki added a commit to fpm-git/bull that referenced this issue Sep 9, 2020
@Rua-Yuki
feat: allow updating job error/attempt info via script
2d7c1fe 
Affects the following scripts:
  - moveToDelayed
  - moveToFinished
  - retryJob

Allows for the `attemptsMade`, `stacktrace` and `message` fields to be
updated through Lua scripts, rather than requiring a separate operation
(or multiple operations) to set these fields.

With the possibility of setting these fields within the context of a
single script, we avoid the need to craft a transaction containing both
HMSET and EVAL(SHA) commands to achieve atomicity. By shedding the need
for a transaction, we are able to avoid redis/ioredis#536 and thus solve
a common problem seen when targetting Redis clusters.

This isn’t necessarily an ideal means of solving this problem, given
we’re violating DRY principles by copying the same logic across
scripts. It’d be ideal if the underlying ioredis restriction was
removed (in a manner which doesn’t entail the performance cost of
resubmitting scripts each time).
Rua-Yuki added a commit to fpm-git/bull that referenced this issue Sep 9, 2020
@Rua-Yuki
feat: allow updating job error/attempt info via script
522fe4d 
Affects the following scripts:
- moveToDelayed
- moveToFinished
- retryJob

Allows for the `attemptsMade`, `stacktrace` and `message` fields to be
updated through Lua scripts, rather than requiring a separate operation
(or multiple operations) to set these fields.

With the possibility of setting these fields within the context of a
single script, we avoid the need to craft a transaction containing both
HMSET and EVAL(SHA) commands to achieve atomicity. By shedding the need
for a transaction, we are able to avoid redis/ioredis#536 and thus solve
a common problem seen when targetting Redis clusters.

This isn’t necessarily an ideal means of solving this problem, given
we’re violating DRY principles by copying the same logic across
scripts. It’d be ideal if the underlying ioredis restriction was
removed (in a manner which doesn’t entail the performance cost of
resubmitting scripts each time).
Rua-Yuki added a commit to fpm-git/bull that referenced this issue Sep 9, 2020
@Rua-Yuki
feat: allow updating job error/attempt info via script (OptimalBits#758)
6794ffa 
Affects the following scripts:
- moveToDelayed
- moveToFinished
- retryJob

Allows for the `attemptsMade`, `stacktrace` and `message` fields to be
updated through Lua scripts, rather than requiring a separate operation
(or multiple operations) to set these fields.

With the possibility of setting these fields within the context of a
single script, we avoid the need to craft a transaction containing both
HMSET and EVAL(SHA) commands to achieve atomicity. By shedding the need
for a transaction, we are able to avoid redis/ioredis#536 and thus solve
a common problem seen when targetting Redis clusters.

This isn’t necessarily an ideal means of solving this problem, given
we’re violating DRY principles by copying the same logic across
scripts. It’d be ideal if the underlying ioredis restriction was
removed (in a manner which doesn’t entail the performance cost of
resubmitting scripts each time).
Rua-Yuki added a commit to fpm-git/bull that referenced this issue Sep 9, 2020
@Rua-Yuki
feat: allow updating job error/attempt info via script (OptimalBits#758)
bf8c0c1 
Affects the following scripts:
- moveToDelayed
- moveToFinished
- retryJob

Allows for the `attemptsMade`, `stacktrace` and `message` fields to be
updated through Lua scripts, rather than requiring a separate operation
(or multiple operations) to set these fields.

With the possibility of setting these fields within the context of a
single script, we avoid the need to craft a transaction containing both
HMSET and EVAL(SHA) commands to achieve atomicity. By shedding the need
for a transaction, we are able to avoid redis/ioredis#536 and thus solve
a common problem seen when targetting Redis clusters.

This isn’t necessarily an ideal means of solving this problem, given
we’re violating DRY principles by copying the same logic across
scripts. It’d be ideal if the underlying ioredis restriction was
removed (in a manner which doesn’t entail the performance cost of
resubmitting scripts each time).
@ShogunPanda ShogunPanda mentioned this issue Sep 30, 2020
Merged
AVVS pushed a commit that referenced this issue Oct 23, 2020
@ShogunPanda @mcollina @AVVS
feat: Add autopipeline for commands and allow multi slot pipelines.
aba3c74 
Fixes #536.

Co-Authored-By: Matteo Collina <[email protected]>
@joebowbeer joebowbeer mentioned this issue Oct 23, 2020
Closed
ioredis-robot pushed a commit that referenced this issue Oct 23, 2020
@semantic-release-bot
chore(release): 4.19.0 [skip ci]
5497b09 
# [4.19.0](v4.18.0...v4.19.0) (2020-10-23)

### Bug Fixes

* Ensure delayed callbacks are always invoked. ([d6e78c3](d6e78c3))

### Features

* Add autopipeline for commands and allow multi slot pipelines. ([aba3c74](aba3c74)), closes [#536](#536)
@ioredis-robot
Copy link
Collaborator

🎉 This issue has been resolved in version 4.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@manast
Copy link
Author

manast commented Nov 19, 2020

FYI, I just upgraded to 4.19.0 and some of Bull tests are failing. I need to investigate more, but for now I will need to lock ioredis to 4.18.0 in order to not break bull users environments.

@snyk-bot snyk-bot mentioned this issue Dec 5, 2020
Closed
@snyk-bot snyk-bot mentioned this issue Jan 6, 2021
Closed
@cc-prodsec cc-prodsec mentioned this issue Mar 2, 2021
Closed
@n0rick
Copy link

n0rick commented May 28, 2022 •
edited
Loading

sorry. useless comment.

@rahmed-hartree rahmed-hartree mentioned this issue Nov 7, 2023
Open
janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this issue Mar 1, 2024
@janus-dev87
chore(release): 4.19.0 [skip ci]
deef58a 
# [4.19.0](redis/ioredis@v4.18.0...v4.19.0) (2020-10-23)

### Bug Fixes

* Ensure delayed callbacks are always invoked. ([d6e78c3](redis/ioredis@d6e78c3))

### Features

* Add autopipeline for commands and allow multi slot pipelines. ([aba3c74](redis/ioredis@aba3c74)), closes [#536](redis/ioredis#536)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
pinned released
Projects
None yet
Milestone
No milestone