Add Lua Scripting support to nri-redis #109
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
The core will be in Redis.Script
Turns out error messages are important
This is failing bc I misunderstood `eval` in Redis Eval is: EVAL script numkeys key key key arg arg arg Keys here is not the NAME of the argument, as I thought, but rather an input parameter in full There's no named arguments in Redis. I assumed that by reading examples from the C# API Client in the Rate Limiting implementation docs. Args and Keys are referenced like KEYS[1] KEYS[2] KEYS [3] and ARGV[1] ARGV[2] ARGV[3] Let's fix that in the next few commits
omnibs
force-pushed
the
phx-1356-add-eval-api-to-nri-redis
branch
from
e7e63a7 to
4e3fb2a
Compare
Splits up keys and arguments, uses KEYS[1] and ARGV[1] inside luaScript
This required a RedisResult instance for Text
Redis provides a faster API for running scripts, where you: - SCRIPT LOAD "the script" - EVALSHA "script sha1" n_keys [keys] [args] I can't implement this because we'd need to restrict `doRawQuery`. `doRawQuery` is generic for both instances of `RedisCtx` in Hedis: - Transactions: `RedisCtx RedisTx Queued` - Regular queries: `RedisCtx Redis (Either Reply)` I'll need a different approach for this.
Allows us to do the `evalsha -> script load -> evalsha` flow
The Handler eval is better. The only thing it doesn't support is auto-expire, but I'm skeptical it's a good idea to use that with Lua scripts arbitrarily.
omnibs
changed the title
micahhahn
requested changes
May 28, 2024
nri-redis/test/Spec/Redis/Script.hs
Outdated
Comment on lines
120
to
124
| -- We can't test for compile-time errors, but manually test our helpful error message, uncomment | ||
| -- the lines below: | ||
| -- Test.test "compilation error" <| \_ -> | ||
| -- [script|${123}|] | ||
| -- |> Expect.equal "Doesn't matter, this won't compile" |
There was a problem hiding this comment.
Sooo, we can in fact test for compile-time errors :)
The basic idea is that you call your function normally (not in a splice), and then there are tools in the Language.Haskell.TH.TestUtils module which let you run the Q monad and test the result.
You might want to take a look at what I did in nri-postgresql/test/Enum.hs way back when I added postgres enum support.
There was a problem hiding this comment.
Also, let's pair if this is something you would want to add!
…-incoherentinstances Patch to avoid using `IncoherentInstances`
I ended up not needing this, but I noticed it was missing and failing tests for nri-postgresql locally
In CI I think the postgres user gets created somehow. Locally it doesn't
micahhahn
approved these changes
May 30, 2024
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.
Add an API to do Lua scripting to
Nri.Redis.We have a new Handler function
eval, which does the recommended dance:EVALSHA ${sha1}LOAD SCRIPT ${theScript}EVALSHA ${sha1}The failure case only happens:
I initially tried to do this as a
Redis.Api/Internal.Query, but it didn't work. If I understand semantics of type classes correctly, it won't unlessInternal.Queryis aMonad, not anApplicative.Hedis uses a different
RedisResponsetypeclass for scripting results, which has its own decoding thingy. I wrote a few orphaned instances for common data types, likeText,Intand().Notes:
evalAPI supports namespacing of keysevalAPI does not support auto-extending key expiry