Rewrite _move helper in Lua (to add Redis 7 support)
#284
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tasktiger/lua/move_task.lua
Outdated
| elseif mode == "min" then | ||
| zadd_update_min({ key }, { score, member }) | ||
| elseif mode == "max" then | ||
| zadd_update_max({ key }, { score, member }) |
There was a problem hiding this comment.
Sounds like we could just call zadd with gt?
There was a problem hiding this comment.
Though lt and gt are not supported by Redis 4.0, so we should think whether we can drop support here.
(nx and xx are supported though)
There was a problem hiding this comment.
replying just once, applies to all 😄
two reasons I decided against it:
- NX and XX were added in v3.0.2, LT & GT were added in 6.2.0. we're currently running TT on 3.2 so we can't use LT>
- I wanted to keep the implementation as close as possible to the original, just rewritten to Lua. That's because this change is risky as is, I don't want to do optimizations and refactors in the same step as well
There was a problem hiding this comment.
an example for the 2nd point - the native implementation of NX, XX, GT could be subtly different from what we do in our scripts. It could slip through and work most of the time but cause bugs or corruption down the line.
thomasst
reviewed
Jun 16, 2023
thomasst
approved these changes
Jun 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #268
In order to not need the
execute_pipelinescript that doesn't work on Redis >= 6.2.7 (and also on earlier versions with security patches backported, like on AWS ElastiCache), I rewrote the_move()helper in Lua directly.Added the ability to include existing scripts as local functions when loading a script from file in order to facilitate code reuse.
Any suggestions how to improve the Lua code? For example Redis doesn't know how to pass bools and nulls to Lua so I had to convert them to strings, any better options?