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

Warn on boot if maxmemory policy is not 'noeviction' #4752

Merged
merged 1 commit into from Dec 2, 2020
Merged

Warn on boot if maxmemory policy is not 'noeviction' #4752

merged 1 commit into from Dec 2, 2020

Conversation

odlp
Copy link
Contributor

@odlp odlp commented Dec 2, 2020

This PR proposes logging a warning when the CLI boots if the maxmemory policy is not 'noeviction'.

Inspired by Nate Berkopec's tweet the other day (I've also had clients who had this misconfiguration):

Running Sidekiq from a Redis datastore with a volatile eviction policy is probably the most common Sidekiq error I see in the wild w/clients. Switch to noeviction, you want Sidekiq to fail very loudly when you run out of memory, not silently drop jobs.

@odlp
Copy link
Contributor Author

odlp commented Dec 2, 2020

Test failure on Ruby 2.5 appears to be unrelated. It's a temporal assertion which failed (CI running slow?):

Failure:
API::with an empty database#test_0024_can enumerate retries [/home/runner/work/sidekiq/sidekiq/test/test_api.rb:484]:
Expected |1606907616.4180453 - 1606907616.3701653| (0.047879934310913086) to be <= 0.02.

rails test /home/runner/work/sidekiq/sidekiq/test/test_api.rb:472

@@ -62,6 +62,11 @@ def run(boot_app: true)
ver = Sidekiq.redis_info["redis_version"]
raise "You are connecting to Redis v#{ver}, Sidekiq requires Redis v4.0.0 or greater" if ver < "4"

maxmemory_policy = Sidekiq.redis_info["maxmemory_policy"]
if maxmemory_policy != "noeviction"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There will be a lot of these in development. I'd suggest you only print this if env != "development".

Copy link
Contributor Author

@odlp odlp Dec 2, 2020

Choose a reason for hiding this comment

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

Taking a look at redis.conf in various versions, I believe noeviction is the default:

But appreciate that it's a somewhat meaningless check in development, apart from the educational value. I'll add the additional condition 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, if it’s the default in the brew formula then I’m ok with it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mperham mperham merged commit c4c01f8 into sidekiq:master Dec 2, 2020
@mperham
Copy link
Collaborator

mperham commented Dec 2, 2020

Thank you, great idea!

@Dantemss
Copy link

I have a question about why this warning is necessary when using volatile-* policies:
If the maxmemory policy is volatile-* then keys without a TTL will never be evicted...
So I imagine those policies would work fine unless Sidekiq was setting a TTL on important keys or if the client somehow managed to configure redis-rb to add a TTL to all keys.
I've seen that configuration option on the Rails cache side, but I don't think redis-rb itself supports it.
So what would be causing those keys to expire?

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

3 participants