Conversation
Fixes rails#32610 Lua scripts in redis are *blocking*, meaning that no other client can execute any commands while the script is running. See https://redis.io/commands/eval#atomicity-of-scripts. This results in the following exceptions once the number of keys is sufficiently large: BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. This commit replaces the lua-based implementation with one that uses `SCAN` and `DEL` in batches. This doesn't block the server. The primary limitation of `SCAN`, i.e. potential duplicate keys, is of no consequence here, because `DEL` ignores keys that do not exist.
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
| private_constant :DELETE_GLOB_LUA | ||
| # The maximum number of entries to receive per SCAN call. | ||
| SCAN_BATCH_SIZE = 1000 | ||
| private_constant :SCAN_BATCH_SIZE |
There was a problem hiding this comment.
Is this specifically chosen? Should we measure it? Should we be able to tune it?
There was a problem hiding this comment.
It is arbitrary (we want a value that doesn't result in too many requests, while keeping the individual responses small, e.g. under 100 KiB), I don't think it's necessary to make it tuneable until somebody asks (at which point they can add an option).
| start, keys = c.scan(start, match: pattern, count: SCAN_BATCH_SIZE) | ||
| c.del(*keys) unless keys.empty? | ||
| break if start == "0" | ||
| end |
There was a problem hiding this comment.
Could move the break to a loop condition, e.g.
cursor = "0"
begin
cursor, keys = c.scan(cursor, …)
c.del(*keys) if keys.any?
end until cursor == "0"| start = "0" | ||
| pattern = namespace_key(matcher, options) | ||
| # Fetch keys in batches using SCAN to avoid blocking the Redis server. | ||
| while true |
There was a problem hiding this comment.
Can we satisfy the https://redis.io/commands/scan#guarantee-of-termination scenario of never terminating if matching keys are being added more quickly than we can consume them?
There was a problem hiding this comment.
In theory, yes (no way it around that I know of).
In practice, I don't think that's possible:
SCANis fast, 1 million records in less than a second on 2011 consumer laptop hardware.- Generating cache keys is not fast (requires doing something that needs caching first) and at least 1,000 cache writes per second would need to be happening assuming 1 million keys and 2011-level laptop hardware (likely many more, because modern server hardware is faster).
This is not even taking into account that SCAN is not guaranteed to return all the elements added during iteration.
| raise ArgumentError, "Only Redis glob strings are supported: #{matcher.inspect}" | ||
| end | ||
| redis.with do |c| | ||
| start = "0" |
There was a problem hiding this comment.
Redis SCAN calls this the cursor. Nice to stick with upstream terminology.
|
@jeremy Thanks for reviewing! Can you also backport this to the previous Rails versions? I expect it will apply with no conflicts. |
Fixes rails#32610. Closes rails#32614. Lua scripts in redis are *blocking*, meaning that no other client can execute any commands while the script is running. See https://redis.io/commands/eval#atomicity-of-scripts. This results in the following exceptions once the number of keys is sufficiently large: BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. This commit replaces the lua-based implementation with one that uses `SCAN` and `DEL` in batches. This doesn't block the server. The primary limitation of `SCAN`, i.e. potential duplicate keys, is of no consequence here, because `DEL` ignores keys that do not exist. cherry-pick ef2af62
4 participants
Fixes #32610
Lua scripts in redis are blocking, meaning that no other client can execute any commands while the script is running. See https://redis.io/commands/eval#atomicity-of-scripts.
This results in the following exceptions once the number of keys is sufficiently large:
This commit replaces the lua-based implementation with one that uses
SCANandDELin batches. This doesn't block the server.The primary limitation of
SCAN, i.e. potential duplicate keys, is of no consequence here, becauseDELignores keys that do not exist.