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

Implement (most of) the rest of redis.* properties and methods for Lua scripts #1050

Merged
merged 43 commits into from
Mar 7, 2025

Conversation

kevin-montrose
Copy link
Contributor

@kevin-montrose kevin-montrose commented Feb 25, 2025

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 config LuaLoggingMode
  • redis.set_repl - this raises an unconditional error, as it doesn't make much sense in Garnet
  • redis.replicate_commands - this unconditionally returns true, as in Redis 7+
  • redis.breakpoint and redis.debug - these raise unconditional errors, as implementing them is both weird and a big ask
  • redis.acl_check_cmd - the lion's share of the PR, see below
  • redis.setresp - also a chunky part of the PR, see below
  • REDIS_VERSION and REDIS_VERSION_NUM
  • Global variables forbidden
  • Global functions forbidden

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:

  • LOG_DEBUG -> Debug
  • LOG_VERBOSE -> Info
  • LOG_NOTICE -> Warning
  • LOG_ERROR -> Error

By default redis.log(...) writes to whatever ILogger is configured. LuaLoggingMode can be used to configure as: Enable (default), Silent (call validated, but logs not written), or Disable (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 incorrect Arity values in there), and creates a dummy RESP request array to shove into RespServerSession.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, and ZSCORE) 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 for redisLuaParity, and 0c8bd6ffffa26b3b0cfd3bceaa5afa0890fae79c for main. I'm including just the Native,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

Method Params Mean Error StdDev Median Allocated
ResetParametersSmall Native,None 6.256 us 0.4919 us 1.427 us 6.100 us 352 B
ResetParametersLarge Native,None 6.204 us 0.5221 us 1.515 us 5.700 us 448 B
ConstructSmall Native,None 201.292 us 5.0428 us 13.889 us 198.500 us 1008 B
ConstructLarge Native,None 217.891 us 8.8451 us 25.802 us 209.100 us 3448 B
CompileForSessionSmall Native,None 32.873 us 1.4644 us 4.178 us 32.850 us 1024 B
CompileForSessionLarge Native,None 119.605 us 6.9591 us 20.410 us 120.950 us 688 B

redisLuaParity

Method Params Mean Error StdDev Median Allocated
ResetParametersSmall Native,None 7.269 us 0.5090 us 1.477 us 7.650 us 688 B
ResetParametersLarge Native,None 6.689 us 0.4603 us 1.343 us 6.800 us 1024 B
ConstructSmall Native,None 280.155 us 8.1001 us 23.110 us 277.400 us 504 B
ConstructLarge Native,None 293.043 us 7.6313 us 21.896 us 292.800 us 3616 B
CompileForSessionSmall Native,None 38.793 us 1.9871 us 5.605 us 38.100 us 352 B
CompileForSessionLarge Native,None 121.113 us 6.0109 us 17.629 us 114.600 us 736 B

LuaScripts

Some jitter here, but no real change.

main

Method Params Mean Error StdDev Gen0 Allocated
Script1 Native,None 124.4 ns 0.99 ns 0.87 ns - -
Script2 Native,None 152.7 ns 0.52 ns 0.43 ns 0.0002 24 B
Script3 Native,None 235.1 ns 2.25 ns 1.88 ns 0.0005 32 B
Script4 Native,None 243.0 ns 2.53 ns 2.36 ns - -

redisLuaParity

Method Params Mean Error StdDev Gen0 Allocated
Script1 Native,None 132.3 ns 1.72 ns 1.52 ns - -
Script2 Native,None 159.4 ns 2.16 ns 2.02 ns 0.0002 24 B
Script3 Native,None 236.8 ns 1.86 ns 1.74 ns 0.0005 32 B
Script4 Native,None 221.3 ns 1.39 ns 1.30 ns - -

ScriptOperations

Some jitter here as well, but not real changes.

main

Method Params Mean Error StdDev Gen0 Gen1 Gen2 Allocated
ScriptLoad Native,None 82.36 us 1.578 us 2.107 us - - - 9600 B
ScriptExistsTrue Native,None 18.85 us 0.187 us 0.175 us - - - -
ScriptExistsFalse Native,None 17.52 us 0.108 us 0.096 us - - - -
Eval Native,None 61.40 us 0.285 us 0.267 us - - - -
EvalSha Native,None 26.38 us 0.200 us 0.167 us - - - -
SmallScript Native,None 54.09 us 0.254 us 0.198 us - - - -
LargeScript Native,None 4,117.96 us 58.913 us 55.107 us - - - 5 B
ArrayReturn Native,None 114.84 us 2.295 us 2.732 us - - - -

redisLuaParity

Method Params Mean Error StdDev Median Gen0 Gen1 Gen2 Allocated
ScriptLoad Native,None 78.79 us 0.278 us 0.247 us 78.87 us - - - 9600 B
ScriptExistsTrue Native,None 19.69 us 0.235 us 0.220 us 19.70 us - - - -
ScriptExistsFalse Native,None 17.72 us 0.258 us 0.241 us 17.60 us - - - -
Eval Native,None 62.02 us 0.392 us 0.348 us 62.08 us - - - -
EvalSha Native,None 27.17 us 0.192 us 0.170 us 27.09 us - - - -
SmallScript Native,None 52.28 us 0.410 us 0.383 us 52.18 us - - - -
LargeScript Native,None 4,010.61 us 45.194 us 40.064 us 4,008.89 us - - - -
ArrayReturn Native,None 116.23 us 0.729 us 0.609 us 116.00 us - - - -

kevin-montrose and others added 30 commits February 24, 2025 15:01
…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).
@kevin-montrose kevin-montrose marked this pull request as ready for review March 3, 2025 19:18
Copy link
Collaborator

@badrishc badrishc left a 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.

@kevin-montrose kevin-montrose merged commit 7a04574 into main Mar 7, 2025
15 checks passed
@kevin-montrose kevin-montrose deleted the redisLuaParity branch March 7, 2025 20:48
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.

5 participants