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

Use evalsha for scripts #77

Merged
merged 1 commit into from
Oct 4, 2020
Merged

Conversation

yosiat
Copy link
Contributor

@yosiat yosiat commented Jul 13, 2020

Lazy loading the scripts: trying with evalsha if it fails on “NO SCRIPT” we will run eval (which will load the script) and next call will use evalsha.

#76

@yosiat yosiat mentioned this pull request Jul 13, 2020
@coveralls
Copy link

coveralls commented Jul 13, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 13cfd0d on yosiat:evalsha into e90e80a on mike-marcacci:master.

@yosiat
Copy link
Contributor Author

yosiat commented Jul 16, 2020

@mike-marcacci can you please look at this PR?

redlock.js Outdated
return server.evalsha(script.hash, args, (err, result) => {
if(err !== null && err.message.startsWith("NOSCRIPT")) {
// load the script, but don't wait for it, on the next call it will be available
server.script('load', script.value);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call regular eval later you don't need to call load, as it automatically caches the script.
https://redis.io/commands/eval#script-cache-semantics

Executed scripts are guaranteed to be in the script cache of a given execution of a Redis instance forever. This means that if an EVAL is performed against a Redis instance all the subsequent EVALSHA calls will succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed.

Lazy loading the scripts: trying with evalsha if it fails on “NO SCRIPT” we will load the script (using eval) so next run will have evalsha.
@yosiat
Copy link
Contributor Author

yosiat commented Aug 17, 2020

@mike-marcacci can you please CR this PR?

@yosiat
Copy link
Contributor Author

yosiat commented Oct 1, 2020

@mike-marcacci if you can, please look at this PR :)
It can reduce bandwidth to redis a lot...

@mike-marcacci mike-marcacci merged commit b6d2628 into mike-marcacci:master Oct 4, 2020
@mike-marcacci
Copy link
Owner

Sorry for the delay - it's been a busy year 😬

Hoping to cut a new release shortly.

@yosiat
Copy link
Contributor Author

yosiat commented Oct 4, 2020

@mike-marcacci thanks a lot, totally understandable :)

@yosiat yosiat deleted the evalsha branch October 4, 2020 07:35
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.

None yet

4 participants