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

API suggestion: add ROM for Lua+exec #2346

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

API suggestion: add ROM for Lua+exec #2346

wants to merge 6 commits into from

Conversation

mgravell
Copy link
Collaborator

@mgravell mgravell commented Jan 16, 2023

only Lua implemented for now, to discuss API shape

  • add ReadOnlyMemory<T> API for Lua
  • add single-key API for Lua
  • fix incorrect use of non-RO onward call for RO Lua in key-prefix path
  • add ReadOnlyMemory<object> for exec (modules)? or try to be more clever? maybe a new readonly struct RedisKeyOrValue which is a known union of the two, and a ReadOnlyMemory<RedisKeyOrValue> ? (at the moment we take object because the API can't know the positions of key vs non-key args, and we need to know which are keys for routing - but using object forces a lot of boxing)
  • add / fixup tests
  • (open question) any other APIs where we should consider ReadOnlyMemory<T> APIs? mget? hmget? del? touch? mset? hset?
  • (open question) add on IServer path? not sure IServer needs super optimize
  • (open question) change [Loaded]LuaScript to exploit this with leased buffers? (I am not a fan of that API)

in particular note the lease approach used in the key-prefix path

discussion topic: any usage passing the literals null or default as the current RedisKey[] keys arg is now ambiguous due to the single-key overload (RedisKey has some operators); this is source-breaking, but is trivially fixable. How much do we care about that? in reality, the number of Lua scripts that don't take any keys can be approximated as zero, so I do not anticipate this impacting much code.

- add single-key API for Lua
- fix incorrect use of non-RO onward call for RO Lua in key-prefix path
@NickCraver
Copy link
Collaborator

Poking at this locally, I think the main problem I see is the current null/default case, I can see that likely breaking someone. Specifically the set of:

        db.ScriptEvaluate(script, default); // BOOM
        db.ScriptEvaluate(script, null); // BOOM

I was thinking if we removed the default defaults for the ReadOnlyMemory overloads and forced the user to pass default there it'd improve things but the most basic conflict is early on: that RedisKey (from null->string->RedisKey implicit) and RedisKeys[]? parameter set which I'm not sure how we'd get around - that's always going by be ambiguous with a single argument and I'd imagine that is used by someone today.

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.

2 participants