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

Address misleading documentation #2444

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SonnyRR
Copy link
Contributor

@SonnyRR SonnyRR commented Apr 25, 2023

Explicitly mention that if previously loaded 'Lua' script was evaluated by it's 'SHA-1' hash, and that hash doesn't exist on the redis server anymore (e.g. server reboot, SCRIPT FLUSH, etc) - it will be reloaded with the 'SCRIPT LOAD' redis command.

Resolves #2443

@SonnyRR
Copy link
Contributor Author

SonnyRR commented Apr 25, 2023

Might be a good idea to update the markdown documentation page as well with this PR. I'll commit new changes for it later.

@SonnyRR SonnyRR force-pushed the improvement/loaded-script-documentation branch from 658ef1f to cf917c8 Compare April 25, 2023 18:05
@SonnyRR
Copy link
Contributor Author

SonnyRR commented Apr 25, 2023

This whole abstraction: LoadedLuaScript seems kind of weird to me in it's current state. I guess it's not deprecated due to compatibility concerns. But I was wondering is there another reason that I might be unaware of ? Running SCRIPT EXIST sha1 before evaluating the script by it's hash does not sound ideal to me, but let's say we're always checking for a single script the complexity should be O(1). Are there any gains to be made if a check was introduced to evaluate the script by it's hash - if it exists, and if not - evaluate it with EVAL.

Update: This seems to be irrelevant.

docs/Scripting.md Outdated Show resolved Hide resolved
@SonnyRR SonnyRR force-pushed the improvement/loaded-script-documentation branch 2 times, most recently from 7e9834a to c6893e3 Compare April 25, 2023 18:30
@mgravell
Copy link
Collaborator

Thing is... I'm not sure it is wrong. It should still hit the local script cache against our endpoint instance, so once we've seen it load: it should switch to the hash version. I'd want to investigate this via redis-cli monitor before updating any docs (noting that it will still use the full script initially)

@SonnyRR
Copy link
Contributor Author

SonnyRR commented Apr 27, 2023

@mgravell Thanks for the heads up, I saw that EVAL was being used after a SCRIPT LOAD command was executed to load a script that previously was cached. When I saw that the changes to the loaded script abstraction, that do not invoke the ScriptEvaluate(byte[] hash, ...) overload, I presumed it always used the EVAL redis command. I'm still new to this codebase, so I've missed that local cache instance. Can you also verify that this is the behavior you're seeing in the monitor logs? Unfortunately, I cannot share examples, but I can probably setup a sandbox project that replicates the problem.

I'll re-word my changes to make it clear what the current behavior is.

@SonnyRR SonnyRR force-pushed the improvement/loaded-script-documentation branch from c6893e3 to 08d7db4 Compare May 3, 2023 07:32
Add remarks for the evaluation methods of the 'LoadedLuaScript' abstraction which explicitly state that if a SHA-1 hash, of a previously loaded Lua script, was not found in the server cache - it will be reloaded with 'SCRIPT LOAD'. The first evaluation of the reloaded script will be carried out by the 'EVAL' command, but any subsequent evaluations will use 'EVALSHA'.
@SonnyRR SonnyRR force-pushed the improvement/loaded-script-documentation branch from 08d7db4 to b1d1ac0 Compare May 3, 2023 07:39
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.

Misleading documentation about the evaluation of server loaded Lua scripts
2 participants