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

Updated purge() and updateMetrics() LUA-scripts to be more deterministic #1501

Closed
wants to merge 3 commits into from

Conversation

Jespercal
Copy link

After being unable to use the 'horizon:clear' without getting the Exception:
"ERR Error running script (call to f_281c23c60c642d42fdedcc1acc8adc7317dd75a5): @user_script:18: @user_script: 18: Write commands not allowed after non deterministic commands. Call redis.replicate_commands() at the start of your script in order to switch to single commands replication mode."

I figured I would try to resolve it, so I updated the scripts to be more deterministic.

In purge(), the cursor is set to 0 and the batch-size is 100, but this could be changed.

Jesper Callesen added 3 commits September 26, 2024 12:53
Updated purge() and updateMetrics() to be more deterministic, to avoid exception.
@Jespercal Jespercal marked this pull request as draft September 26, 2024 11:17
@Jespercal Jespercal marked this pull request as ready for review September 26, 2024 11:17
@taylorotwell
Copy link
Member

Do we have tests for this?

@Jespercal
Copy link
Author

test_it_removes_recent_jobs_when_queue_is_purged() in Feature/RedisJobRepositoryTest tests the command.
It doesn't throw any errors here on Github, but if I run it without the code changes locally, it throws the same exception as above. After the code change, the test passes.

@Jespercal
Copy link
Author

Jespercal commented Oct 1, 2024

I did some digging, and it all comes down to the fact that I'm using Redis 3.2.9 locally through MAMP PRO. In that version, script replication is verbatim by default, so I'm getting the exception.
As of version 5.2, effect is the default replication mode, so the exception is never thrown, because it doesn't matter for the effect mode.

All in all, the script doesn't really need to be deterministic for Redis versions above 5.2, but it doesn't hurt that it is. And it also adds backward compatibility with older versions of Redis, like the one I'm using.

If the code changes seem a little extreme, an alternative could be adding "redis.replicate_commands()" at the beginning. It changes the mode from verbatim to effect, where Redis is verbatim as default, like in <5.2 versions of Redis. In newer versions of Redis, it does nothing and just returns true.

@taylorotwell
Copy link
Member

If it's only for older versions will just table it for now.

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

Successfully merging this pull request may close these issues.

2 participants