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.

Sign up for GitHub

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

Jump to bottom

Reload redis scripts on reconnect #143

Merged
merged 3 commits into from
Jul 11, 2018
Merged

Reload redis scripts on reconnect #143

merged 3 commits into from
Jul 11, 2018

Conversation

lpfeup
Copy link
Contributor

@lpfeup lpfeup commented Jun 22, 2018

fixes #100

ofirnk
ofirnk reviewed Jun 22, 2018
View reviewed changes
Copy link
Contributor

@ofirnk ofirnk left a comment

Choose a reason for hiding this comment

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

Simple and to the point 🎉

src/main/java/net/greghaines/jesque/worker/WorkerImpl.java Outdated
try {
loadRedisScripts();
} catch (IOException e) {
LOG.error("Failed to load redis scripts", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Failed to reload Lua scripts after reconnect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ofirnk
ofirnk approved these changes Jun 22, 2018
View reviewed changes
Copy link
Contributor

@ofirnk ofirnk left a comment

Choose a reason for hiding this comment

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

maybe write few words whether and how this was tested?
Anyway, It lgtm 👍

@lpfeup
Copy link
Contributor Author

lpfeup commented Jun 22, 2018 •
edited
Loading

To test this, I basically initialized and started a WorkerImpl and then restarted the redis instance (redis implicitly flushes the script cache on restart).

Without this fix, WorkerImpl would terminate with a JedisNoScriptException while polling the queues after a redis restart.
With this fix, the worker keeps running after a redis restart.

EDIT: tested with redis docker v3.2.12

@ofirnk
Copy link
Contributor

ofirnk commented Jun 22, 2018

Cool, sounds good 👏

@ofirnk ofirnk mentioned this pull request Jun 25, 2018
Closed
@gresrun gresrun merged commit 5ef2310 into gresrun:master Jul 11, 2018
@bp-FLN bp-FLN mentioned this pull request Aug 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@argvk argvk argvk approved these changes

@ofirnk ofirnk ofirnk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

If redis restarts, lua scripts aren't reloaded
4 participants
@lpfeup @ofirnk @argvk @gresrun