-
Notifications
You must be signed in to change notification settings - Fork 557
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
Implement (most of) the rest of redis.* properties and methods for Lua scripts #1050
Merged
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
…be disabled (either silently, or raising and error) if desired
* SETKEEPTTL (no other flags) should always return ok. It doesn't care about NX or XX * Simplify GETSET --------- Co-authored-by: prvyk <[email protected]> Co-authored-by: Tal Zaccai <[email protected]>
* expose diskless replication parameters * refactor/cleanup legacy ReplicaSyncSession * add interface to support diskless replication session and aof tasks * core diskless replication implementation * expose diskless replication API * adding test for diskless replication * update gcs extension to clearly mark logging progress * fix gcs dispose on diskless attach, call dispose of replicationSyncManager, add more logging * complete first diskless replication test * fix iterator check for null when empty store * fix iterator for object store cluster sync * add simple diskless sync test * cleanup code * replica fall behind test * wip * register cts at wait for sync completion * add db version alignment test * avoid using close lock for leader based syncing * truncate AOF after streaming checkpoint is taken * add tests for failover with diskless replication * fix formatting and conversion to IPEndpoint * fix RepCommandsTests * dispose aofSyncTask if failed to add to AofSyncTaskStore * overload dispose ReplicaSyncSession * explicitly dispose gcs used for full sync at replicaSyncSession sync * dispose gcs once on return * code cleanup * update tests to provide more context logging * add more comprehensive logging of syncMetadata * add timeout for streaming checkpoint * add clusterTimeout for diskless repl tests * some more logging * cleanup and refactor code * truncate AOF only when main-memory-replication is switched on * adding logging for cancellation when streaming * split checkpoint commit marker to allow for disk checkpoints * update sync metadata log message * add progress based timeout implementation * deprecate main-memory-replication
…be disabled (either silently, or raising and error) if desired
…s and inner sessions. Lots of testing, comparing against Redis. Fixes RESP3 behavior of HGETALL, SDIFF, ZSCORE (more fixes for other commands needed in separate PR).
badrishc
approved these changes
Mar 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, some minor comments.
TalZaccai
reviewed
Mar 7, 2025
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.
Knocking these out now, as refactoring to work around
longjmp
is going to be onerous and tricky - and I want a good set of tests for all functionality before doing that.Adds:
redis.pcall
redis.sha1hex
redis.LOG_DEBUG
,redis.LOG_VERBOSE
,redis.LOG_NOTICE
,redis.LOG_WARNING
redis.log
- along with new configLuaLoggingMode
redis.set_repl
- this raises an unconditional error, as it doesn't make much sense in Garnetredis.replicate_commands
- this unconditionally returns true, as in Redis 7+redis.breakpoint
andredis.debug
- these raise unconditional errors, as implementing them is both weird and a big askredis.acl_check_cmd
- the lion's share of the PR, see belowredis.setresp
- also a chunky part of the PR, see belowREDIS_VERSION
andREDIS_VERSION_NUM
I'll do a separate change for the runtime libraries not currently supported, as I also think we want some ability to configure what's actually imported.
Details
redis.log
Mapping between Redis log levels and .NET log levels is:
By default
redis.log(...)
writes to whateverILogger
is configured.LuaLoggingMode
can be used to configure as:Enable
(default),Silent
(call validated, but logs not written), orDisable
(call validated, and then error raised).Silent
may seem strange, but is meant for the case where Garnet is hosted as a service which doesn't want to pollute its logs with user logs normally, but wants the option to gather those logs.Silent
allows log lines to be placed, but ignored most of the time.redis.acl_check_cmd
This method checks if the current user has permission to run a RESP command.
Trick is, you don't have to pass the whole command (in the sense of all arguments, or even subcommands). Most of our code assumes we have a whole command available to parse or otherwise validate.
So this implementations fakes it. It consults
RespCommandsInfo
just enough to figure out minimum argument counts (I had to fix a few incorrectArity
values in there), and creates a dummy RESP request array to shove intoRespServerSession.ParseCommand
via a new helper.Some small changes were necessary to make all that work. I changed the ACL tests to also exercise this method, to give us more confidence in correctness.
redis.setresp
This method changes how LUA <-> RESP values are mapped depending on RESP version. This affects both the results of scripts, and the results of
redis.(p)call
, so it's rather involved.As part of testing this, I fixed a few commands (
HGETALL
,SDIFF
, andZSCORE
) whose response types change under RESP3 to correctly return new types. I strongly suspect there are a lot more commands that aren't accounting for RESP3, but will do that work in a separate PR.Forbidden global variable and function declarations
In
main
we allow global declarations, which can carry over between script invocations. Redis forbids this, and it's a giant footgun - these globals are only present in a particular session, and their lifetimes are heavily implementation dependent.These are now forbidden, which is technically a breaking change. Anyone depending on this behavior was already subtly broken, so I think it's fine.
Note that Redis appears to implement this by patching Lua - I did it in pure Lua, which imposes higher overhead but lets us keep using vanilla Lua 5.4.
Benchmarks
Since this change adds a fair amount to the bootstrapping of a Lua VM, I expected some loss in initial script loading - which is observed. Preventing global functions and variables had the risk of a general regression in Lua performance, but that does not seem to have happened.
All benchmarks are as of
77c1587bfa894e1884b2e5275ee3e798b1e3b9c8
forredisLuaParity
, and0c8bd6ffffa26b3b0cfd3bceaa5afa0890fae79c
formain
. I'm including just theNative,None
results for conciseness.LuaRunnerOperations
As expected, we're giving up a bit in construction (when the VM is created) and compilation (when various global functions are initialized). Worst case loss of ~40% (for
ConstructSmall
) - some of this is simply unavoidable, and this is typically amoratized over multiple script invocations.main
redisLuaParity
LuaScripts
Some jitter here, but no real change.
main
redisLuaParity
ScriptOperations
Some jitter here as well, but not real changes.
main
redisLuaParity